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

Update eth-typing dependency #177

Merged
merged 6 commits into from Nov 25, 2019
Merged

Conversation

@njgheorghita
Copy link
Contributor

njgheorghita commented Nov 21, 2019

What was wrong?

  • Remove local typing module in favor of using eth-typing types
  • Added mypy.ini configuration file

To-Do

  • Clean up commit history

Cute Animal Picture

image

fixes #176
fixes #28

@njgheorghita njgheorghita force-pushed the njgheorghita:use-eth-typing branch 3 times, most recently from 4e16d7d to 3f9fa4a Nov 21, 2019
@njgheorghita njgheorghita force-pushed the njgheorghita:use-eth-typing branch from 3f9fa4a to 8e5abb8 Nov 21, 2019
@njgheorghita njgheorghita requested review from cburgdorf and pipermerriam Nov 21, 2019
Copy link
Contributor

cburgdorf left a comment

Nice! Some places look suspicious though. I'd recommend to try running mypy on Trinity with these changes and see if they conflict and if yes, which one is at fault. I'm 99 % certain the ExtendedDebugLogger typing is not correct and will cause errors in Trinity.

eth_utils/conversions.py Show resolved Hide resolved
eth_utils/decorators.py Show resolved Hide resolved
eth_utils/decorators.py Outdated Show resolved Hide resolved
return logging.getLogger(name)


def get_extended_debug_logger(name: str) -> ExtendedDebugLogger:
def get_extended_debug_logger(name: str) -> logging.Logger:

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 21, 2019

Contributor

This feels wrong to me. Since this returns a ExtendedDebugLogger and ExtendedDebugLogger derives from logging.Logger the return type should be ExtendedDebugLogger. I'm 99 % certain this will cause problems at every place that calls logger = get_extended_debug_logger('foo') and then tries to call logger.debug2("meh"). It will rightfully yell that debug2is not a method onlogging.Loggerbecause we are hiding the fact that what we return is in fact anExtendedDebugLogger`

This comment has been minimized.

Copy link
@njgheorghita

njgheorghita Nov 21, 2019

Author Contributor

Yeah, the problem I'm having is with get_logger() -> logging.Logger. With...

def get_extended_debug_logger(name: str) -> ExtendedDebugLogger:

and

def get_logger(name: str, logger_class: Type[logging.Logger] = None) -> TLogger:
errors....
eth_utils/logging.py:68: error: Incompatible return value type (got "Logger", expected "TLogger")
eth_utils/logging.py:84: error: Incompatible return value type (got "Logger", expected "TLogger")

or with

def get_logger(name: str, logger_class: Type[logging.Logger] = None) -> logging.Logger:
errors...
eth_utils/logging.py:88: error: Incompatible return value type (got "Logger", expected "ExtendedDebugLogger")

Even though to me, the first option with TLogger looks correct, but for some reason that I'm probably missing it's not inferencing the Logger bound on TLogger correctly?

This comment has been minimized.

Copy link
@njgheorghita

njgheorghita Nov 21, 2019

Author Contributor

I ended up just casting the return value in get_extended_debug_logger() to ExtendedDebugLogger - so everything works, but there might be a better solution.

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 21, 2019

Contributor

Yeah, fine by me. The bound looks correct. Probably worth following up on this because it suggests get_logger does not preserve the type as we want it to. Anyway, the cast is an acceptable solution for now imho.

@njgheorghita njgheorghita force-pushed the njgheorghita:use-eth-typing branch from 3dbe157 to 0f8ba4d Nov 21, 2019
Copy link
Contributor

cburgdorf left a comment

LGTM

Copy link
Member

pipermerriam left a comment

Seconding what @cburgdorf said. I'd recommend trying this in both PyEVM and Trinity to see if it behaves as expected.

@@ -107,16 +108,18 @@ def to_checksum_address(value: AnyStr) -> ChecksumAddress:
Makes a checksum address given a supported format.
"""
norm_address = to_normalized_address(value)
address_hash = encode_hex(keccak(text=remove_0x_prefix(norm_address)))
address_hash = encode_hex(keccak(text=remove_0x_prefix(HexStr(norm_address))))

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Nov 21, 2019

Member

It seems like to_normalized_address should return a HexStr type... so this wrapping seems fishy to me..

This comment has been minimized.

Copy link
@njgheorghita

njgheorghita Nov 21, 2019

Author Contributor

It will once ethereum/eth-typing#21 lands.

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Nov 21, 2019

Member

Maybe include a comment linking to that issue/PR here in the code.

This comment has been minimized.

Copy link
@njgheorghita

njgheorghita Nov 22, 2019

Author Contributor

It turns out I was wrong, thanks to @cburgdorf helping me out with an explanation - but to_normalized_address() returns a HexAddress, and remove_0x_prefix() requires a HexStr. So the cast here will be necessary unless we want to relax constraints, i.e...

remove_0x_prefix(value: Union[HexStr, HexAddress]) -> HexStr:
add_0x_prefix(value: Union[HexStr, HexAddress]) -> HexStr:

But i'm not sure how I feel about this solution, since any HexAddress types will get lost b/c of the logic inside those functions - thoughts?

eth_utils/address.py Show resolved Hide resolved
eth_utils/logging.py Outdated Show resolved Hide resolved
eth_utils/logging.py Outdated Show resolved Hide resolved
eth_utils/logging.py Outdated Show resolved Hide resolved
eth_utils/logging.py Outdated Show resolved Hide resolved
@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

njgheorghita commented Nov 22, 2019

Thanks @pipermerriam & @cburgdorf - tested this locally against py-evm and trinity and mypy seems to be happy with the changes made - but tests are broken on both b/c eth-keys uses some of the types from the eth_utils.types module that was removed in this pr in favor of eth-typing types.

@pipermerriam

This comment has been minimized.

Copy link
Member

pipermerriam commented Nov 22, 2019

@njgheorghita that means we can't just remove eth_utils.types as it would be a breaking change. You can mirror the types from eth-typing there and add a deprecation warning on module import that indicates the module has been deprecated and will be removed in the next major version bump.

@njgheorghita njgheorghita merged commit d05279c into ethereum:master Nov 25, 2019
10 checks passed
10 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: lint-py36 Your tests passed on CircleCI!
Details
ci/circleci: lint-py37 Your tests passed on CircleCI!
Details
ci/circleci: lint-py38 Your tests passed on CircleCI!
Details
ci/circleci: py35 Your tests passed on CircleCI!
Details
ci/circleci: py36 Your tests passed on CircleCI!
Details
ci/circleci: py36-typing1 Your tests passed on CircleCI!
Details
ci/circleci: py37 Your tests passed on CircleCI!
Details
ci/circleci: py38 Your tests passed on CircleCI!
Details
ci/circleci: pypy3 Your tests passed on CircleCI!
Details
@njgheorghita njgheorghita deleted the njgheorghita:use-eth-typing branch Nov 25, 2019
)

warnings.warn(
"The ethpm_utils.typing module will be deprecated in favor "

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Nov 25, 2019

Member

s/ethpm_utils/eth_utils/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.