Remove doctest dependency on ethtoken #1395
Conversation
|
@pipermerriam / @carver / @kclowes Reopened this from #1202. @pipermerriam - I addressed this comment in the second commit here: Before
After
|
| @@ -109,7 +109,7 @@ pytest -n 4 -f --maxfail=1 | |||
|
|
|||
| 2. Execute `tox` for the tests | |||
|
|
|||
| There are multiple [components](https://github.com/ethereum/web3.py/blob/master/.travis.yml#L53) of the tests. You can run test to against specific component. For example: | |||
| There are multiple [components](https://github.com/ethereum/web3.py/blob/master/.circleci/config.yml#L144) of the tests. You can run test to against specific component. For example: | |||
There was a problem hiding this comment.
Updated this to a similar line to what was linked before as the project no longer uses Travis. Previously pointed to this:
https://github.com/ethereum/web3.py/blob/v4.9.0/.travis.yml#L53
| .. doctest:: | ||
|
|
||
| >>> from ethtoken.abi import EIP20_ABI |
There was a problem hiding this comment.
I still think it's important for people to be able to run the examples locally, which this breaks. At least, we should link to the ABI, as we discussed in: #1202 (comment)
There was a problem hiding this comment.
Do you mean put a literal link to the ABI? Like as a comment?
There was a problem hiding this comment.
Right, a link so that documentation readers can fill in the EIP20_ABI variable when they run it in their own console.
There was a problem hiding this comment.
Does this look fine? Also, just curious, but is it redundant to set nonce = 0 in the setup?
There was a problem hiding this comment.
Yeah, looks redundant to me.
There was a problem hiding this comment.
I was going to remove it, but it actually seems it's set because we don't actually make the call below:
>>> nonce = w3.eth.getTransactionCount('0x5ce9454909639D2D17A3F753ce7d93fa0b9aB12E') # doctest: +SKIP
|
@carver the nonce assignment isn't actually called in the
Should be good to go once the build finishes! |
|
@carver @pipermerriam bumping this 🎉 |
| .. doctest:: | ||
|
|
||
| >>> from ethtoken.abi import EIP20_ABI | ||
| # When running locally, execute the statements found in the file linked below to load the EIP20_ABI variable. | ||
| # See: https://github.com/carver/ethtoken.py/blob/v0.0.1-alpha.4/ethtoken/abi.py |
There was a problem hiding this comment.
cc @njgheorghita you might update this sometime with an optional way to load the ABI using an EthPM package.
What was wrong?
** Reopened PR from #1202 **
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