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

allow eth.filter to return appropriate sync/async filter types #2645

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Sep 12, 2022

Calls to AsyncEth.filter were returning sync filter types

How was it fixed?

Updated associated plumbing, added testing

Cute Animal Picture

image

@pacrob pacrob requested review from fselmo and kclowes September 12, 2022 21:33
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Module has a property called is_async already. I think we could use that to differentiate instead of adding another is_async property on Method.

Also, I haven't tested this, but it looks like you could also combine the filter_wrapper with async_filter_wrapper if you wanted to. That's more of a stylistic thing though :) Otherwise, this looks good!

LogFilter,
]:

if type(module).__name__ == "Eth":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if this is just a wip and you don't want feedback, but could you check for module.is_async here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem - feedback always welcome, especially if I've asked for review. Will try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated, ready for re-review.

@pacrob pacrob force-pushed the method-class-async-flag branch 2 times, most recently from 3cf4b51 to c77aa42 Compare September 14, 2022 19:08
@pacrob pacrob force-pushed the method-class-async-flag branch from c77aa42 to 2a21a0f Compare September 14, 2022 19:18
@pacrob pacrob force-pushed the method-class-async-flag branch from 2a21a0f to 61b52b6 Compare September 14, 2022 19:22
@pacrob pacrob changed the title add async flag to Method class allow eth.filter to return appropriate sync/async filter types Sep 14, 2022
@pacrob pacrob requested a review from kclowes September 14, 2022 19:41
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Nice! LGTM!

@pacrob pacrob merged commit 21c23cd into ethereum:asyncify-filters Sep 14, 2022
@pacrob pacrob deleted the method-class-async-flag branch September 14, 2022 20:02
pacrob pushed a commit that referenced this pull request Oct 7, 2022
* update method_formatters.py to select the appropriate sync/async filter type
pacrob pushed a commit that referenced this pull request Oct 24, 2022
* update method_formatters.py to select the appropriate sync/async filter type
pacrob pushed a commit that referenced this pull request Nov 21, 2022
* update method_formatters.py to select the appropriate sync/async filter type
pacrob pushed a commit that referenced this pull request Dec 2, 2022
* update method_formatters.py to select the appropriate sync/async filter type
pacrob pushed a commit to pacrob/web3.py that referenced this pull request Dec 8, 2022
…eum#2645)

* update method_formatters.py to select the appropriate sync/async filter type
pacrob pushed a commit that referenced this pull request Dec 19, 2022
* update method_formatters.py to select the appropriate sync/async filter type
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.

2 participants