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

Tidy mypy and tox config #614

Merged
merged 15 commits into from May 9, 2020
Merged

Conversation

bhrutledge
Copy link
Contributor

@bhrutledge bhrutledge commented May 3, 2020

Towards #231

  • Use explicit ignore_missing_imports. I think this could help us keep an eye on projects that might add annotations in the future. It will also cause mypy to fail if somebody adds a library that's missing them, which seems like an opportunity to make a thoughtful choice about how to proceed.

  • Remove monketype from tox.ini, since it's no longer needed

  • Add skip_install = True to formatting and linting tox environments, to speed them up

  • Reworked tox and Travis config: Tidy mypy and tox config #614 (comment)

@bhrutledge
Copy link
Contributor Author

Ah, interesting. mypy failed with:

twine/__init__.py:32: error: Cannot find implementation or library stub for module named 'importlib_metadata'

Which I didn't catch, because I've been using py38 locally. I'll fix that.

@bhrutledge
Copy link
Contributor Author

I tweaked the Travis configuration to do type checking with the latest and oldest supported Python versions. @jaraco what do you think?

@bhrutledge bhrutledge requested a review from jaraco May 3, 2020 18:44
@jaraco
Copy link
Member

jaraco commented May 3, 2020

Thanks for putting this together and including me.

I have a slight preference to make the CI routines as generic as possible and the tox.ini as portable as possible. By adding separate build steps across different python versions but only on Travis, it adds to the complication of those builds, raising the bar for moving to another platform. It additionally increases the variance of this project's Travis file compared to other projects.

That said, this project isn't part of any collection of projects, and the typing stage is already a deviation from the procedures I follow on other projects, so it's really not a big deal. Moreover, I don't have a lot of experience with typing or mypy, so I don't have any better recommendation to make at this point.

You have my endorsement.

I do have one question regarding the typing - is it the case that there are situations where testing on latest would catch errors not on oldest and vice versa? If one could reasonably expect just one of those runs to catch 99.9% of issues, I'd prefer to use just that one.

@jaraco
Copy link
Member

jaraco commented May 3, 2020

For an example of how I approach this need in my other projects, I've committed jaraco/skeleton@7455f2f. Note how this few-line change will enable mypy testing for users running tox and for all CI environments (Travis, Appveyor, Azure Pipelines) and platforms (Windows, macOS, Linux) across hundreds of projects.

@bhrutledge
Copy link
Contributor Author

bhrutledge commented May 4, 2020

Is it the case that there are situations where testing on latest would catch errors not on oldest and vice versa?

Yes. In our case, importlib_metadata was flagged as a missing import in py36 and py37, but not in py38, because there we use importlib.metadata.

I have a slight preference to make the CI routines as generic as possible and the tox.ini as portable as possible.
Note how this few-line change will enable mypy testing for users running tox and for all CI environments.

I still prefer to use tools like mypy and black directly, but these comments got me thinking about ways to simplify. To that end, I've reworked tox.ini to include tests, type checking, and linting in testenv, while maintaining distinct environments for the last two. I also took the opportunity to generate both HTML and text reports for coverage and mypy, because I've found them useful.

Due to the scope of these changes, I'm going to request review from the other maintainers.

Open questions:

  • What do y'all think of this approach?

  • Should there be a distinct test environment, such that testenv is just the union of test, typing, and lint? That would allow running just the tests.

  • Are the reports too much? Personally, I find the text versions of coverage/mypy kind of noisy (see Travis for an example), but the HTML is handy. I'm happy to disable one or both completely, and let folks generate them on demand. FWIW, on my machine, generating all the reports adds ~1s to the test run time.

  • Should testenv set ignore_errors = True? That would allow all the commands to run and report failures, though I think it would imply disabling the reports, because it doesn't make sense to run those on failure.

One alternative to including mypy in testenv would be to run it multiple times, but specifying different versions:

[testenv:mypy]
skip_install = True
ignore_errors = True
deps =
    mypy
    lxml
commands =
    mypy --python-version 3.6
    mypy --python-version 3.7
    mypy --python-version 3.8 --html-report mypy --txt-report mypy twine

This seems to work regardless of the basepython version.

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@bhrutledge
Copy link
Contributor Author

Alrighty, I've reverted my experiment. Thanks for indulging me; I've got better insight into how y'all use tox.

Remaining changes for approval:

  • 87e64cf Remove mypy from lint testenv, to avoid confusion about which to use, simplify the tox config, and align it with the Travis config
  • 7ff608c Generate text and HTML reports for mypy and coverage; I don't feel strongly about this, and would happily remove any or all of these

@bhrutledge
Copy link
Contributor Author

@jaraco @sigmavirus24 Any more thoughts before I merge this? Specifically re: #614 (comment).

@sigmavirus24
Copy link
Member

Ship it

@jaraco
Copy link
Member

jaraco commented May 8, 2020

No objections.

@bhrutledge
Copy link
Contributor Author

Thanks, y'all. A potential future enhancement would be to use tox's new support for generative section names:

diff --git a/tox.ini b/tox.ini
index 14738c2..e330d4a 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,6 @@
 [tox]
 minversion = 2.4
-envlist = lint,types,py{36,37,38},docs
+envlist = lint,types-py{36,38},py{36,37,38},docs
 
 [testenv]
 deps =
@@ -46,7 +46,7 @@ commands =
     black --check --diff twine/ tests/
     flake8 twine/ tests/
 
-[testenv:types]
+[testenv:types-py{36,38}]
 skip_install = True
 deps =
     mypy

@bhrutledge bhrutledge merged commit d5c6a98 into pypa:master May 9, 2020
@bhrutledge bhrutledge deleted the 231-tidy-config branch May 9, 2020 09:46
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.

None yet

3 participants