Skip to content

Conversation

@shreyav
Copy link
Contributor

@shreyav shreyav commented Jan 14, 2025

No description provided.

uma/uma.py Outdated

async def _run_http_get_async(url: str) -> str:
async with ClientSession() as session:
async with session.get(url) as response: # pyre-ignore [16]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the pyre error

uma/uma.py:152:19 Undefined attribute [16]: `aiohttp.client.ClientSession` has no attribute `get`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might need to pin to 3.10 on aiohttp due to pyre incompatibility aio-libs/aiohttp#8463

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that fixed it, thanks!

@shreyav shreyav marked this pull request as draft January 14, 2025 16:18
@shreyav shreyav marked this pull request as ready for review January 17, 2025 00:46
return public_key


async def fetch_public_key_for_vasp_async(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about using the gen_* convention here like we do internally? I don't have a strong preference, but was curious if anyone else does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yunyuyunyu do you have any thoughts?
from quick googling it doesn't seem like a common convention so I almost prefer not to, but also not a strong preference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I actually don't see many external libraries using prefix gen for their async functions. Hmm, @vdurmont do you have a preference?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly for external stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks all -- going to leave as is then!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd lean towards keeping our consistency and use gen_. I acknowledge it's not widely used in the outside world, but it doesn't hurt at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update!

@jklein24
Copy link
Contributor

Force merging because the 3.8 test won't run anymore after this.

@jklein24 jklein24 merged commit f0b407a into main Jan 21, 2025
2 checks passed
@jklein24 jklein24 deleted the feat/async-pubkey-cache branch January 21, 2025 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants