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

Upgrade typehints to support disallow-untyped-defs flag #282

Merged
merged 5 commits into from
Feb 19, 2020

Conversation

Peilonrayz
Copy link
Contributor

Core Changes

Since more typehints are desired, #276, I have added typehints to support the --disallow-untyped-defs flag. The change set is large, as I've added annotations so all functions have them. For the most part the change set only includes:

  • Add the required typing imports.
  • Add function annotations to all functions.
  • Add and update some type comments.
  • Ignore typehints, "type: ignore", which can't be solved by the above solutions.

Even though all functions have been hinted, this is by no means a complete solution. There are lots of errors being ignored, with a significant amount being around how you bind things like python to a Callable. There's also a lack of generics, and there's a lot of Any.

Errors

  • When running nox --session tests-3.6, with 3.6.0, one of the test_command.py tests fails. With the final line of the original traceback being:

    sqlite3.OperationalError: Safety level may not be changed inside a transaction
    

    This only happens when using nox. Manually changing into the venv and running pytest doesn't cause that error.

  • For me the coverage test fails. I'm unsure if this is due to not using Conda, or because I'm using x.y.0 for all Python versions.

  • The docs session doesn't work for me on Windows. The command rm can't be found.

  • Black and isort can't agree on how the imports should be formatted in _parametrize.py, manifest.py and sessions.py. I have manually ran Black to get the lint session to not error locally.

These errors are making it hard to see if anything basic is blocking the PR.

Additional Changes

I've tried to keep the changes strictly to the core changes. However, I have deviated from that plan slightly. And so below are all the changes that I have made that are to achieve the goal, but aren't addressed above.

Non-typehint

  • .gitignore

    • Mypy creates a cache under .mypy_cache/.
    • Visual Studio Code puts configurations under .vscode/.
  • noxfile.py
    I have added the --disallow-untyped-defs flag when mypy is run.

typehint

Imports

  • nox/manifest.py
    Import argparse and nox.sessions.Session.

  • nox/sessions.py
    Import argparse and nox.manifest.Manifest.

  • nox/tasks.py
    Import importlib.machiniery and types.

  • nox/workflow.py
    Import argparse.

Type Aliases

These are to reduce duplicate definitons, and to make the types easier to read.

  • nox/_parameterize.py
    Add ArgValue.

  • nox/registry.py
    Add Python.

Other

  • nox/logger.py
    Use typing.cast to ensure logger gets the correct type.

  • nox/virtualenv.py
    Add an explicit return None to locate_via_py.

    The added return can be replaced with --no-warn-no-return flag to mypy instead.

  • nox/_typing.py
    I have added this file as a small central interface to typing and typing_extensions. This is as typing_extensions supplies the backport and experimental typing features. Currently it, _typing.py, only contains code to interface with Python 3.5.0-3 and Python 3.6.0-1.

    I have used __all__ to ignore flake8 as NoReturn is being used, but is being used in a string annotation - as it will not be defined in Python 3.5.0 if the user doesn't have typing_extensions installed.


This is a fairly big PR. I'm happy to split it up if desired.

@theacodes
Copy link
Collaborator

Thank you so much for doing this!

When running nox --session tests-3.6, with 3.6.0, one of the test_command.py tests fails. With the final line of the original traceback being

Not sure what's going on here. I'll try to research when I'm home next week, but if you solve it in the meantime that's awesome!

For me the coverage test fails. I'm unsure if this is due to not using Conda, or because I'm using x.y.0 for all Python versions.

Yeah, coverage won't report everything locally if you don't have conda. This is expected. You can push here and see if CI passes.

The docs session doesn't work for me on Windows. The command rm can't be found.

I wouldn't sweat it. CI will test docs.

Black and isort can't agree on how the imports should be formatted in _parametrize.py, manifest.py and sessions.py. I have manually ran Black to get the lint session to not error locally.

Maybe update the isort to this config:

[settings]
multi_line_output=3
include_trailing_comma=True
force_grid_wrap=0
use_parentheses=True
line_length=88

That seems to be what the black documentation recommends.

@Peilonrayz
Copy link
Contributor Author

Ok, I'll try get everything green here. I was hoping Travis would run, but it seems I needed to resolve the conflicts first. So I'll start trying to fix any issues it reports.

I don't think I'll be able to solve the issue with 3.6.0, as I barely know how I'd debug it - and my first step made the issue disappear. I can give it a go once I get the PR to light up green. But I don't think I'll be of any use here.

@Peilonrayz
Copy link
Contributor Author

Further changes

  • Merge changes. I made a couple of blunders so it's split over two commits.

  • Update isort so it's not fighting black. Thanks @theacodes your solution worked straight away.

  • Ignore coverage on typing only code. This comes in two parts:

    • I've added if _typing.TYPE_CHECKING: as an exclude line. This is because any code that follows it will never run.

    • I have flat out ignored nox/_typing.py.

      I think this is the best solution, as if anyone is running the tests on 3.5.0 than they are likely to get different coverage results than 3.5.latest. Additionally I don't think coverage is a good metric for the code in the file.


CI seems to be happy with the above changes. I'm still getting the error with 3.6.0, however it was erroring before I started work on this PR, so I hope it's thought of a separate issue.

I won't be able to fix anything blocking this for the next day or so. But I should be able to after that.

@theacodes
Copy link
Collaborator

theacodes commented Feb 8, 2020 via email

@Peilonrayz
Copy link
Contributor Author

No problem, there's no rush. Have a good weekend!

@theacodes theacodes merged commit 1bd08bc into wntrblm:master Feb 19, 2020
@theacodes
Copy link
Collaborator

Thank you, @Peilonrayz!

@Peilonrayz
Copy link
Contributor Author

Thanks for the merge 😁

I looked into the 3.6.0 issue today and found it to not be a problem with nox. It looks like either pytest-cov or coverage is the one with the issue. Sorry to mess you about like this 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants