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

Raise exceptions rather than return None in all jsonrpc calls #1218

Merged
merged 3 commits into from
Feb 28, 2019
Merged

Raise exceptions rather than return None in all jsonrpc calls #1218

merged 3 commits into from
Feb 28, 2019

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Jan 18, 2019 •
edited
Loading

What was wrong?

#722 (comment)

I propose that we change web3.eth.getBlock, web3.eth.getTransaction and other similar methods to no longer return None, but instead to raise an exception. The current API encourages/requires return value checking which is not a pattern I think we should be promoting.

How was it fixed?

Raised a ValueError if return value from Manager._make_request() is None

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the raise-exceptions-in-web3-eth branch from e94441c to 2ac48f6 Compare January 18, 2019 10:41
@njgheorghita
Copy link
Contributor Author

@pipermerriam What other web3.eth methods are candidates for this update?

@njgheorghita njgheorghita force-pushed the raise-exceptions-in-web3-eth branch 2 times, most recently from 80a0da7 to 0cb275b Compare January 18, 2019 10:47
@carver carver mentioned this pull request Feb 21, 2019
Closed
22 tasks
@njgheorghita njgheorghita changed the title Raise exceptions rather than return None in some eth calls Raise exceptions rather than return None in all jsonrpc calls Feb 26, 2019
@njgheorghita njgheorghita force-pushed the raise-exceptions-in-web3-eth branch from 0cb275b to 5f3eb60 Compare February 26, 2019 19:58
@njgheorghita njgheorghita force-pushed the raise-exceptions-in-web3-eth branch from e8d6fc3 to 56bea98 Compare February 26, 2019 20:19
@njgheorghita njgheorghita requested a review from kclowes February 26, 2019 20:28
@njgheorghita
Copy link
Contributor Author

@pipermerriam ping for review

kclowes
kclowes approved these changes Feb 27, 2019
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 looks good to me @njgheorghita!

njgheorghita
njgheorghita commented Feb 28, 2019
View reviewed changes
web3/manager.py
@@ -107,6 +110,9 @@ def request_blocking(self, method, params):
if "error" in response:
raise ValueError(response["error"])

if response['result'] is None:
raise ValueError(f"The call to {method} did not return a value.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kclowes @dylanjw I'm not greatly familiar with the async gameplan for web3 - but it occurred to me that if we are raising exceptions for jsonrpc calls that return None in synchronous requests, then we should have the same behavior in the async requests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

dylanjw
dylanjw approved these changes Feb 28, 2019
View reviewed changes
web3/manager.py
@@ -107,6 +110,9 @@ def request_blocking(self, method, params):
if "error" in response:
raise ValueError(response["error"])

if response['result'] is None:
raise ValueError(f"The call to {method} did not return a value.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@njgheorghita njgheorghita merged commit 406d5e7 into ethereum:master Feb 28, 2019
@njgheorghita njgheorghita deleted the raise-exceptions-in-web3-eth branch February 28, 2019 03:08
@njgheorghita njgheorghita restored the raise-exceptions-in-web3-eth branch February 28, 2019 04:01
pipermerriam
pipermerriam reviewed Mar 4, 2019
View reviewed changes
web3/_utils/module_testing/eth_module.py
@@ -485,8 +485,8 @@ def test_eth_getBlockByHash(self, web3, empty_block):
assert block['hash'] == empty_block['hash']

def test_eth_getBlockByHash_not_found(self, web3, empty_block):
block = web3.eth.getBlock(UNKNOWN_HASH)
assert block is None
with pytest.raises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that I'm late to the party on this one, but I think that this should be a custom exception for each object type.

  • BlockNotFound
  • TransactionNotFound (use this for receipt fetching too)
  • anything else?

cc @njgheorghita

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pipermerriam pipermerriam pipermerriam left review comments

@kclowes kclowes kclowes approved these changes

@dylanjw dylanjw dylanjw 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.
4 participants
@njgheorghita @pipermerriam @kclowes @dylanjw