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

Drop py3.5 support, support py3.8, fix lint test #97

Merged
merged 3 commits into from
May 4, 2020

Conversation

carver
Copy link
Contributor

@carver carver commented May 1, 2020

What was wrong?

fix #96

Also, drop py3.5 support (breaking change, requires major version bump, of course!)

How was it fixed?

Update tox, circle-ci etc.

While doing that, notice that the lint tests were probably broken. Hm, nope, the lint tests were working befdore! It's a mystery to me why, I really thought that tox's testenv didn't support partial matches. In fact, it came up pretty recently in another project and the lint test was getting skipped, which I thought was because of a partial-match issue...

Also, had to upgrade hypothesis to work with py3.8. This meant that warnings about long tests turned into errors, so some tests had to be made shorter and/or the deadline extended.

Cute Animal Picture

Cute animal picture

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Any reason you chose not to run the linter across the different python versions? Otherwise 👍

@carver
Copy link
Contributor Author

carver commented May 4, 2020

Any reason you chose not to run the linter across the different python versions? Otherwise

Well at first I thought that lint wasn't running at all because of the [testenv:lint] definition, which does not trigger in the environment py36-lint for example. This was a problem in ethereum/eth-keys#72 which definitely was not running lint tests before. So I was surprised to find out that py-trie was running lint tests despite this partial match problem.

After some digging, I found: .circleci was running the lint environment instead of py36-lint:

py35-lint:
<<: *common
docker:
- image: circleci/python:3.5
environment:
TOXENV: lint
py36-lint:
<<: *common
docker:
- image: circleci/python:3.6
environment:
TOXENV: lint

Which means that lots of confusing things were happening:

  1. a local run of tox -e py36-lint would give you a different result than the circleCI job py36-lint (which was really just tox -e lint but in a py36 container)
  2. there were a bunch of listed environments in tox that were never run in CI
  3. the only thing forcing the correct python was in the circleCI config instead of the tox one

So...

I think the right answer is to just stick with the project template standard and only run one linter:
https://github.com/ethereum/ethereum-python-project-template/blob/e38d2349dd9941ac38cae5ab0402a912092fce2d/tox.ini#L4

If the linter itself is giving different results for the same version of flake8 but different versions of python, then I think we can just recognize that as a bug in flake8. That reduces our CI time & noise. I don't think we get much benefit from re-checking lint in a bunch of different pythons.

@carver carver merged commit ec225d3 into ethereum:master May 4, 2020
@carver carver deleted the drop-py3.5-add-3.8 branch May 4, 2020 22:31
@carver carver mentioned this pull request May 19, 2020
10 tasks
pacrob pushed a commit to pacrob/py-trie that referenced this pull request Oct 29, 2023
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.

Add Python 3.8 support
2 participants