-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
class Web3JsonEncoder(json.JSONEncoder): | ||
def default(self, obj): | ||
if isinstance(obj, AttributeDict): | ||
return {k: v for k, v in obj.items()} |
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 seems like it should be a recursive operation, re-encoding each of the k
and v
values using json.JSONEncoder.default(self, v)
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.
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.
Looks like there is also a linting error. See the CI failure for details. |
Think I addressed @pipermerriam's feedback and fixed the linting issues. Added simple docs as well. |
Thank you! |
What was wrong?
Related to Issue #782
How was it fixed?
Added
Web3.toJSON()
and accompanying tests. Basically, this extends the standardjson.JSONEncoder
to supportHexBytes
andweb3.datastructures.AttributeDict
and adds a static method to theWeb3
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