Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing confirm_address #3424

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Fix missing confirm_address #3424

merged 3 commits into from
Nov 30, 2023

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Nov 28, 2023

This PR fixes the problem and adds a lint to check for it in the future.

for now, it catches the following incorrect function:

async def show_foo() -> Awaitable[None]:
    return show_something_else("foo")

because to correctly show the result, the caller would have to "await (await show_foo())"

(this should either be "async def show_foo() -> None", or "def show_foo() -> Awaitable[None]")
multiple bugreports complain about poor support for setup.cfg
for instance, the load-plugins rule is ignored in setup.cfg
@matejcik matejcik force-pushed the matejcik/check-async-awaitable branch from ad488d7 to 55a0509 Compare November 28, 2023 14:55
@matejcik matejcik marked this pull request as ready for review November 28, 2023 15:23
@matejcik matejcik requested review from mmilata, grdddj and andrewkozlik and removed request for andrewkozlik November 28, 2023 15:23
Copy link
Contributor

@grdddj grdddj left a comment

Choose a reason for hiding this comment

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

NIce job!

The pylint worked fine for me locally, even though the speed is disastrous. Did you consider or try creating flake8 plugin? It seems to be much quicker. Unfortunately, ruff still seems not supporting user plugins. (Sure, pylint runs anyway, but it would be nice to get rid of it, so for possible future plugins)

At the first glance, I am not very happy about the different signatures of confirm_address between TR and TT layout code --- was there any reason to not use the second option async def confirm_address() -> None (which should be possible with return await confirm_blob())?

@matejcik
Copy link
Contributor Author

was there any reason to not use the second option async def confirm_address() -> None (which should be possible with return await confirm_blob())?

the sync option (def foo() -> Awaitable[x]) is always preferable because calling an async function allocates a new awaitable, while calling a synchronous function is free.

am not very happy about the different signatures of confirm_address

yes, the TT functions should be converted to what TR does, but I didn't want to do that here. (and it might be moot anyway, as we'll soon be able to get rid of those functions)

the speed is disastrous.

does it seem to you that it's slower than it was before? pylint is super slow generally but I didn't notice this making it worse

@grdddj
Copy link
Contributor

grdddj commented Nov 29, 2023

Thanks for the clarifications.

the speed is disastrous.

does it seem to you that it's slower than it was before? pylint is super slow generally but I didn't notice this making it worse

I did not notice any worsening either, so it was a complaint about general pylint - this one quick plugin should not make it any slower.

@grdddj grdddj self-requested a review November 29, 2023 10:25
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Awesome, looking forward to writing some more checks.

@matejcik matejcik merged commit c8fa687 into main Nov 30, 2023
56 checks passed
@matejcik matejcik deleted the matejcik/check-async-awaitable branch November 30, 2023 08:28
@trezor trezor deleted a comment from komsunkerdsalung Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants