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

Remove doctest dependency on ethtoken #1202

Closed
wants to merge 1 commit into from
Closed

Remove doctest dependency on ethtoken #1202

wants to merge 1 commit into from

Conversation

stefanmendoza
Copy link
Contributor

@stefanmendoza stefanmendoza commented Jan 12, 2019

What was wrong?

ethtoken has a dependency on web3 and web3 depends on ethtoken for doctest. We should remove this circular dependency and just include the EIP20 ABI in the doctest for now.

See: carver/ethtoken.py#4 (comment)

How was it fixed?

Remove ethtoken dependency and added the EIP20 ABI to the relevant test.

Cute Animal Picture

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

>>> from web3.auto import w3

>>> EIP20_ABI = json.loads('[{"constant":true,"inputs":[],"name":"name","outputs":[{"name":"","type":"string"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"name":"_spender","type":"address"},{"name":"_value","type":"uint256"}],"name":"approve","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"totalSupply","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"name":"_from","type":"address"},{"name":"_to","type":"address"},{"name":"_value","type":"uint256"}],"name":"transferFrom","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"decimals","outputs":[{"name":"","type":"uint8"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"name":"_owner","type":"address"}],"name":"balanceOf","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[],"name":"symbol","outputs":[{"name":"","type":"string"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"name":"_to","type":"address"},{"name":"_value","type":"uint256"}],"name":"transfer","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"_owner","type":"address"},{"name":"_spender","type":"address"}],"name":"allowance","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"_from","type":"address"},{"indexed":true,"name":"_to","type":"address"},{"indexed":false,"name":"_value","type":"uint256"}],"name":"Transfer","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"_owner","type":"address"},{"indexed":true,"name":"_spender","type":"address"},{"indexed":false,"name":"_value","type":"uint256"}],"name":"Approval","type":"event"}]')
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to move this into a .. testsetup:: directive so that the EIP20_ABI variable is available in the doctest code blocks but such that we don't have to have this very large JSON string in our code block. See the examples RST file for an example of how we do this elsewhere.

Copy link
Collaborator

@carver carver Jan 14, 2019

Choose a reason for hiding this comment

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

If you do this, at least link to the ABI in ethtoken. Doc examples are improved by being easily reproducible for the reader. Since the reader can't see testsetup, that impedes reproducibility.

Copy link
Contributor Author

@stefanmendoza stefanmendoza Feb 7, 2019

Choose a reason for hiding this comment

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

@carver getting back to this...

Since ethtoken is being removed in this PR, did you literally mean link to the Github URL in a comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, something like: https://github.com/carver/ethtoken.py/blob/11b92df3de56711a602e28c84033ba64de988b24/ethtoken/abi.py

just so people have a chance of running the code locally, without having to hunt for the ABI. If there's a better source for the ABI, I'm all for it.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

LGTM after changes for Piper

@stefanmendoza
Copy link
Contributor Author

@carver @pipermerriam cool, thanks for the review guys! I'll try to get to this tonight. 👍

@kclowes
Copy link
Collaborator

kclowes commented May 9, 2019

@stefanmendoza correct me if I'm wrong, but it looks like this is waiting for the ABI to be converted into a .. testsetup:: directive? Do you think you'll have time to get to that? I'm happy to do it if not, just let me know!

@stefanmendoza
Copy link
Contributor Author

@kclowes feel free to push this over the finish line! Sorry, I got pulled into other things and this got put on the back burner. Thanks!

Also, thanks to @carver and @pipermerriam. I appreciate the review.

@kclowes - Lemme know if you have any questions for me! 🙏

@kclowes
Copy link
Collaborator

kclowes commented May 13, 2019

Sounds good! No worries!

@stefanmendoza
Copy link
Contributor Author

I had deleted my fork, so I recovered the commit from this PR and addressed the last change request. The new PR can be found here:
#1395

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