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

Adds Web3.toJSON per #782 #1173

Merged
merged 6 commits into from
Feb 11, 2019
Merged

Adds Web3.toJSON per #782 #1173

merged 6 commits into from
Feb 11, 2019

Conversation

mikeshultz
Copy link
Contributor

@mikeshultz mikeshultz commented Dec 21, 2018

What was wrong?

Related to Issue #782

How was it fixed?

Added Web3.toJSON() and accompanying tests. Basically, this extends the standard json.JSONEncoder to support HexBytes and web3.datastructures.AttributeDict and adds a static method to the Web3 object that utilizes it.

This is my first PR for this project, so please tear it apart.

It would probably be good to get some more test cases, as well. In what other ways might a user use Web3.toJSON()? Should there be more supported types? In what ways should it not work?

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

class Web3JsonEncoder(json.JSONEncoder):
def default(self, obj):
if isinstance(obj, AttributeDict):
return {k: v for k, v in obj.items()}
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be a recursive operation, re-encoding each of the k and v values using json.JSONEncoder.default(self, v)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like json.JSONEncoder already handles recursion. default() just needs to provide one to one conversion. I added a test with a nested AttributeDict as verification.

@pipermerriam
Copy link
Member

Looks like there is also a linting error. See the CI failure for details.

@mikeshultz
Copy link
Contributor Author

Think I addressed @pipermerriam's feedback and fixed the linting issues. Added simple docs as well.

@kclowes
Copy link
Collaborator

kclowes commented Feb 11, 2019

Thank you!

@kclowes kclowes merged commit 2ac317e into ethereum:master Feb 11, 2019
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.

3 participants