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

refactor: Use cached_property to cache the show_debug2 #232

Merged
merged 3 commits into from
Oct 3, 2022

Conversation

e3243eric
Copy link
Contributor

@e3243eric e3243eric commented Sep 25, 2022

What was wrong?

Issue #159

How was it fixed?

Replace cached_show_debug2_property with cached_property, and add the cached-property package in install_requires.
cached_property has the untyped issue, so I add the # type: ignore.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@e3243eric e3243eric changed the title refactor: Use cached_property to cache debug2 properties refactor: Use cached_property to cache the show_debug2 Sep 25, 2022
@fselmo
Copy link
Contributor

fselmo commented Sep 26, 2022

First, thanks for tackling this @e3243eric!

At this point this issue is a bit older and I have a few thoughts I wanted to throw around. This strays a bit further from a Good First Issue tag but alas, it may be good to have some conversation around this since I'm sure this will come up again in other repos.

  • cached-property doesn't seem like it has been maintained since mid 2020. I'd rather not introduce another dependency to eth-utils that we may have to get rid of in the future. This is a bit weaker argument as it may not need all that much maintenance.

  • @cached_property has been a decorator since python 3.8 as part of the functools module. If we can use this for python versions 3.8 and above, that would be quite nice. Python 3.6 has already been marked as not being supported and should probably be dropped from eth-utils as well. Python 3.7 is being marked as not supported only 9 months from now according to the link referenced.

With the above in mind, I think I lean towards only requiring this for python versions below 3.8, otherwise importing the functools decorator.

So... in setup.py:

install_requires=[
        "eth-hash>=0.3.1,<0.6.0",
        "eth-typing>=3.0.0,<4.0.0",
        "toolz>0.8.2,<1;implementation_name=='pypy'",
        "cytoolz>=0.10.1,<1.0.0;implementation_name=='cpython'",
        "cached-property>=1.5.2,<2; python_version < '3.8'",  # this is the relevant change
    ],

Then in logging.py:

import sys
if sys.version_info < (3, 8):
    from cached_property import cached_property
else:
    from functools import cached_property

I believe the above should work. Curious on others' thoughts here.

CC: @kclowes, @pacrob

@e3243eric
Copy link
Contributor Author

e3243eric commented Sep 28, 2022

I updated the code like the @fselmo comment, it works, but I got a lint issue.

  • When the python version is 3.8+, I import cached-property from functools module. Lint gets the unused error because warn_unused_ignores of mypy is set.
    eth_utils/logging.py:23: error: unused "type: ignore" comment

  • When the python version is older than 3.8, the cached-property module needs the "type: ignore" comment to pass the lint check.

I can pass the CI because the python version of CI lint is 3.6.

@fselmo
Copy link
Contributor

fselmo commented Sep 28, 2022

I updated the code like the @fselmo comment, it works, but I got a lint issue.

Hmm I think this is OK. We should probably drop 3.6 support and update the lint job to run with the latest supported version but I can handle that in a separate PR. Thanks for making that change and confirming it works well 👍🏼. I'll just wait to see if anyone else has an opinion here before merging. Thanks again.

@kclowes
Copy link
Contributor

kclowes commented Oct 3, 2022

Curious on others' thoughts here.

@fselmo - Yep, this is the solution I would do too.

Thanks for all the PRs @e3243eric!

fselmo added a commit to e3243eric/eth-utils that referenced this pull request Oct 3, 2022
@fselmo fselmo merged commit bbdaf5e into ethereum:master Oct 3, 2022
@e3243eric e3243eric deleted the logging branch October 3, 2022 18:14
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.

3 participants