Conversation
* new issue and pr templates
* convert bash scripts to py
* bump versions in dependencies and ci builds * move tox to [dev] per issue ethereum#34 * move RTD deps pointer into .readthedocs.yml * unpin flake8 add flake8-bugbear to lint deps
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
* repin flake8, bump tox to >=4.0.0 as that's where whitelist was deprecated, misc updates
* template cleanup following initial merge with py-evm * add flake8 pin comment * correct license years * add pin note to mypy
…m#81) * apply template updates found following merge with eth-typing * add build as a dev dependency * remove timeout from pytest.ini, it doesn't do anything without pytest-timeout as a dep
* add updates found when merging template with py-ssz and eth-abi * add wheel and wheel-windows to ci and reorg
* remove testall because it doesnt work
minor formatting updates, remove additional docs to separate pr
…sion bump sphinx version and set py version rtd uses to 3.8
change references to doc to all be docs
Pre-release check for upstream configuration of remote
add `.venv*` to .gitignore
add `.build` to be ignored
fix indent, add sphinx fail_on_warning
gitignore local vs-code settings
* add pre-commit * run pre-commit * skip lint on README.md as it breaks template filling
1d3ea54 to
ae3ed26
Compare
reedsa
left a comment
There was a problem hiding this comment.
This all looks in order, lgtm!
kclowes
left a comment
There was a problem hiding this comment.
thanks for taking this on! A few things I noticed:
- It doesn't look like the core, database, difficulty, or vm tests are running
- It looks like some upper pins were removed. I think I'd rather remove them all at the same time, so if that's too much for this PR, let's put those back in
setup.pyand make an issue to do that another time. - nit: it looks like precommit gets installed in the make lint section, so I don't think you need the separate build step for all the test runs
- there are a few doc errors that could very well be user error :)
|
|
||
| test-all: | ||
| tox | ||
| pytest tests |
There was a problem hiding this comment.
It may be user error, but I'm not able to get pytest tests to run. We could either do pytest tests/core here, or pick a different command. I'm not sure what the best command to pick would be. I'm also not opposed to removing it altogether since I don't think it's super useful, but would want to check with @fselmo first since he's in here most often.
There was a problem hiding this comment.
It needs the fixtures installed. CI does it with git submodule update --init --recursive, but the dev docs don't appear to mention it. Will add. It's in the contributing docs, git clone --recursive .... Installing Cloning that way allows make test run for me. You? Takes forever though 😄
Also, I'm seeing the tests you mention running in the CI "All checks have passed" list below. Where are you seeing that they didn't run?
There was a problem hiding this comment.
Ah, that was likely my problem. I knew there was something funky to do with the submodule, but couldn't remember how to fix that.
If you look at the output in the run tox step here: https://app.circleci.com/pipelines/github/ethereum/py-evm/2555/workflows/fd9eaa7a-2157-4416-bda6-eaea85711132/jobs/172217
It's running in 17s, and I don't see the actual command for the tests (pytest tests/json-fixtures/test_difficulty.py). Looks like it usually takes ~2 minutes.
There was a problem hiding this comment.
tests running correctly again!
|
|
||
| build-docs: clean | ||
| cd docs/; sphinx-build -W -T -E . _build/html | ||
| build-docs: |
There was a problem hiding this comment.
This may also be user error/not having the right things installed, but I'm seeing a bunch of warnings when I run make build-docs. For example:
WARNING: autodoc: failed to import module 'run' from module 'scripts.benchmark'; the following exception was raised: No module named 'checks'~/ef/py-evm/docs/tests.rst: WARNING: document isn't included in any toctree
And:
Document: guides/building_an_app_that_uses_pyevm
------------------------------------------------
**********************************************************************
File "guides/building_an_app_that_uses_pyevm.rst", line 100, in default
Failed example:
print(f"The balance of address {encode_hex(MOCK_ADDRESS)} is {mock_address_balance} wei"
Exception raised:
Traceback (most recent call last):
File "/opt/homebrew/Cellar/python@3.11/3.11.4_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/doctest.py", line 1351, in __run
exec(compile(example.source, filename, "single",
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/eve/ef/py-evm/venv-upgrade-template/lib/python3.11/site-packages/sphinx/ext/doctest.py", line 484, in compile
return compile(code, name, self.type, flags, dont_inherit)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<doctest default[10]>", line 1
print(f"The balance of address {encode_hex(MOCK_ADDRESS)} is {mock_address_balance} wei"
^
SyntaxError: '(' was never closed
**********************************************************************
Are you seeing those too?
There was a problem hiding this comment.
I get the same errors, which makes me wonder why CI docs tests are passing. Looking into.
There was a problem hiding this comment.
Once tests got running again, this turned out to indeed be a ( I didn't close.
setup.py
Outdated
|
|
||
| extras_require = { | ||
| "benchmark": [ | ||
| "termcolor>=1.1.0,<2.0.0", |
There was a problem hiding this comment.
Do we need the upper bound here?
| @@ -30,59 +49,21 @@ | |||
| "test": [ | |||
| "factory-boy==2.11.1", | |||
| "hypothesis>=5,<6", | |||
There was a problem hiding this comment.
Do we want to drop upper pins in this section too for consistency?
There was a problem hiding this comment.
All upper pins gone except for towncrier and hypothesis.
5436241 to
b6cb11c
Compare
edb02f4 to
0602237
Compare
0602237 to
22e502f
Compare
|
I think I've addressed everything you noted, but I'm not sure about:
Could you clarify? |
22e502f to
001df43
Compare
|
You're installing precommit on all test runs, but I think we just need it for the lint runs? And it looks like it may already get installed. From the lint tests - looks like a separate step isn't necessary? Not 100% sure on on that point though: |
convert .format and % strings that pyupgrade can't do drop unnecessary deps, remove unnecessary upper pins remove pre-commit install from ci, it's handled by tox
ef74554 to
87b9415
Compare

What was wrong?
Pull in updates from the python project template, notably:
pre-commitfor lintingmasterbranch tomainRelated to Issue #
Closes #1856 <- needs manual closing
Closes #1942
How was it fixed?
Todo:
Clean up commit history
Add or update documentation related to these changes
Add entry to the release notes
Cute Animal Picture