ethereum / eth-utils

Utility functions for working with ethereum related codebases.
https://eth-utils.readthedocs.io/en/latest/
MIT License
312 stars 148 forks source link

refactor: Use cached_property to cache the show_debug2 #232

Closed e3243eric closed 1 year ago

e3243eric commented 1 year ago

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

Cute Animal Picture

put a cute animal picture link inside the parentheses

fselmo commented 1 year ago

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.

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 commented 1 year ago

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

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

fselmo commented 1 year ago

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 commented 1 year ago

Curious on others' thoughts here.

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

Thanks for all the PRs @e3243eric!