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

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

Merged

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Jan 18, 2019

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
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
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
Copy link
Contributor Author

@pipermerriam ping for review

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!

@@ -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.

@@ -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
@@ -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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants