-
Notifications
You must be signed in to change notification settings - Fork 697
Unify ValidationError
#1194
Unify ValidationError
#1194
Conversation
setup.py
Outdated
| "eth-keys>=0.2.0b3,<1.0.0", | ||
| "eth-typing>=1.1.0,<2.0.0", | ||
| "eth-utils>=1.0.1,<2.0.0", | ||
| "eth-utils==1.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the pin to a specific version here?
c7a51a4 to
d862104
Compare
d862104 to
bdbac0a
Compare
pipermerriam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't comment on all of them but can you go and move the eth_utils imports to be above the eth imports. We like to ensure our imports are grouped by namespace.
eth/chains/shard.py
Outdated
| ) | ||
|
|
||
| from eth.exceptions import ( | ||
| from eth_utils import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import should move up above the previous import so that all of the eth imports remain in the same block.
eth/consensus/pow.py
Outdated
| encode_hex, | ||
| ) | ||
| from eth.exceptions import ( | ||
| from eth_utils import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, move it up above the eth imports
|
|
||
| from eth import constants | ||
| from eth.exceptions import ( | ||
| from eth_utils import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should move up above the eth imports
trinity/sync/common/chain.py
Outdated
| HeaderNotFound, | ||
| ValidationError as EthValidationError, | ||
| ) | ||
| from eth_utils import ValidationError as EthValidationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop this rename and just change all of the places where EthValidationError is raised to just be a ValidationError.
79e16ef to
d18b78e
Compare
What was wrong?
https://github.com/ethereum/py-evm/pull/1089/files#r206404376
As noted in this comment, there is a valid concern that our duplicate
ValidationErrorexceptions could easily be mixed up, causing us to not be catching the exception we think we are catching.Issue Reference: #1125
How was it fixed?
Modify the codebase to use that exception class for all
ValidationErrorexceptions to use the one frometh_utilsCute Animal Picture