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

Complete typing of noxfile.py #9411

Merged
merged 1 commit into from
Feb 20, 2021
Merged

Complete typing of noxfile.py #9411

merged 1 commit into from
Feb 20, 2021

Conversation

jdufresne
Copy link
Contributor

Allows removing a mypy: disallow-untyped-defs=False comment.

To workaround a mypy bug, map(os.path.basename, distribution_files) was changed to use a generator expression. See:
python/mypy#9864

@sbidoul sbidoul added skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Jan 1, 2021
noxfile.py Outdated
@@ -34,6 +32,7 @@


def run_with_protected_pip(session, *arguments):
# type: (nox.Sessions, *str) -> None
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

Copy link
Member

Choose a reason for hiding this comment

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

Why does mypy not blow up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo?

Yes! Thanks for catching.

Why does mypy not blow up here?

Good question. This is due to using pre-commit and the mypy import configuration.

mypy runs in pre-commit which builds a virtual environment that has only mypy installed. It does not have nox installed. Therefore, nox's types aren't really available for mypy. The nox types are also not available in typeshed.

mypy is configured with:

ignore_missing_imports = True

So, when the import fails, mypy treats the module as Any and all its attributes (even ones that don't exist) as Any. I was able to confirm this using reveal_type()

To get around this, I can install third party libraries to the pre-commit virtual environment using the additional_dependencies option. Unfortunately, this adds yet another place to configure dependencies.

noxfile.py Outdated Show resolved Hide resolved
@jdufresne
Copy link
Contributor Author

Changes:

nox is now a dependency in pre-commit. This caught the identified typos a few other typing errors. Some of these have PRs open upstream:

wntrblm/nox#376
wntrblm/nox#377
wntrblm/nox#378

Removed configuration follow_imports = silent. This will help ensure that the same issue doesn't resurface. Now, if there is a missing import during mypy, it will report an error.

stsewd pushed a commit to wntrblm/nox that referenced this pull request Jan 22, 2021
os.chdir() has supported PathLib since Python 3.6.

Discovered while adding types pip's noxfile.py:
pypa/pip#9411
theacodes pushed a commit to wntrblm/nox that referenced this pull request Feb 8, 2021
Fixes errors when running mypy on a project's noxfile.py:

    noxfile.py:42: error: "ProcessEnv" has no attribute "location"

Discovered while adding types pip's noxfile.py:
pypa/pip#9411
Allows removing a "mypy: disallow-untyped-defs=False" comment.

To workaround a mypy bug, map(os.path.basename, distribution_files) was
changed to use a generator expression. See:
python/mypy#9864

To verify correct usage of the nox API, it is now a dependency during
pre-commit mypy runs.

The mypy configuration "follow_imports = silent" allowed erroneous code
to pass, so it has been removed. Now, all imports must be available
during type.
@pradyunsg pradyunsg merged commit b588c58 into pypa:master Feb 20, 2021
@pradyunsg
Copy link
Member

Thanks! ^.^

@jdufresne jdufresne deleted the type-noxfile branch February 20, 2021 13:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants