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.

Sign up for GitHub

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

Jump to bottom

Better support for custom methods for modules #2383

Merged
merged 4 commits into from
Mar 10, 2022
Merged

Better support for custom methods for modules #2383

merged 4 commits into from
Mar 10, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Mar 8, 2022 •
edited
Loading

What was wrong?

How was it fixed?

  • Made attaching methods to a module a little easier. This should be easier to document as well although the documentation in [WIP] Document Method #1797 is still relevant and would work. Good documentation is still needed for these updates, perhaps in a separate PR.

Todo:

Cute Animal Picture

moog_shaved4

kclowes
kclowes reviewed Mar 8, 2022
View reviewed changes
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.

🚀 I just took a quick look on my phone so feel free to take or leave the comments if they don’t make sense!

  • even though the Method isn’t public exactly, I know people are using it as such, so at least for the backport PR, we should make sure we’re not breaking anything. It looked the the signature for Method had changed some
  • We should make sure we can still pass mungers=None in at least one test just to guard against it breaking in the future

web3/__init__.py Show resolved Hide resolved
@fselmo
Copy link
Collaborator Author

fselmo commented Mar 8, 2022 •
edited
Loading 

* even though the Method isn’t public exactly, I know people are using it as such, so at least for the backport PR, we should make sure we’re not breaking anything. It looked the the signature for Method had changed some

* We should make sure we can still pass mungers=None in at least one test just to guard against it breaking in the future

There might be a more elegant way around this if we want this functionality for v5 but as it currently stands I was thinking this might be a strictly v6 (breaking change) PR. The reason being is I think that having a set_as_property flag which defaults to False makes the most sense. And in being explicit when this flag is True, it sets the mungers to the same value that mungers=None used to ([default_munger]). Therefore, when the flag is False (default) and mungers=None (also default), it uses the default mungers for methods ([default_root_munger]) instead.

Any ideas there?

@fselmo fselmo force-pushed the better-support-for-custom-method branch 3 times, most recently from e8828e5 to a3d52f5 Compare March 8, 2022 18:19
@kclowes
Copy link
Collaborator

kclowes commented Mar 9, 2022

Nope, I think that API makes sense. A v6 only feature sounds good to me!

@fselmo fselmo marked this pull request as ready for review March 9, 2022 21:10
@fselmo fselmo changed the title [WIP] Better support for custom methods for modules Better support for custom methods for modules Mar 9, 2022
@fselmo
Add option to attach methods to classes extending from Module class
e6f306c 
* Refactor logic for attaching a `Method` class as a property rather than a method. Instead of implicitly setting `mungers=None`, explicitly set the `is_property` flag on `Method` to `True`. This also facilitates attaching new methods and properties to modules.

* Fix up some tests in test_method.py that were falsely passing to actually test correctly. Add tests for new `is_property` flag for the `Method` class.

* Create `test_module.py` and add tests for `attach_methods()`
@fselmo fselmo force-pushed the better-support-for-custom-method branch from a3d52f5 to e6f306c Compare March 9, 2022 21:14
kclowes
kclowes approved these changes Mar 10, 2022
View reviewed changes
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.

This PR is looking good! I mostly had clarifying comments and nits.

tests/core/method-class/test_method.py Show resolved Hide resolved
tests/core/module-class/test_module.py Outdated Show resolved Hide resolved
web3/__init__.py Show resolved Hide resolved
web3/method.py Outdated Show resolved Hide resolved
web3/method.py Show resolved Hide resolved
tests/core/method-class/test_method.py Show resolved Hide resolved
tests/core/method-class/test_method.py Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Mar 10, 2022
@fselmo
More tests for method class and cleanup from PR ethereum#2383 comments
10397ac
fselmo added a commit to fselmo/web3.py that referenced this pull request Mar 10, 2022
@fselmo
More tests for method class and cleanup from PR ethereum#2383 comments
6dbd26e
@fselmo fselmo force-pushed the better-support-for-custom-method branch from 10397ac to 6dbd26e Compare March 10, 2022 12:23
@fselmo fselmo force-pushed the better-support-for-custom-method branch from 6dbd26e to cb6b4a0 Compare March 10, 2022 12:26
@fselmo
Copy link
Collaborator Author

fselmo commented Mar 10, 2022

@kclowes suggested changes and some more robust testing / validation added in the latest commit. Thanks!

kclowes
kclowes approved these changes Mar 10, 2022
View reviewed changes
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.

LGTM! 🚢

@fselmo fselmo merged commit 061fb66 into ethereum:master Mar 10, 2022
@fselmo fselmo deleted the better-support-for-custom-method branch March 10, 2022 23:04
@wolovim wolovim mentioned this pull request May 10, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kclowes kclowes kclowes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fselmo @kclowes