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

Are more type hints desired? #276

Closed
Peilonrayz opened this issue Jan 29, 2020 · 4 comments
Closed

Are more type hints desired? #276

Peilonrayz opened this issue Jan 29, 2020 · 4 comments

Comments

@Peilonrayz
Copy link
Contributor

I was interested in adding a different virtual environment / package manager to nox, independently. And noticed you don't have many type hints. The noxfile.py runs mypy, so I'm thinking the answer is yes.

Are more type hints / static typing desired?


If you do:

  • Is there anything you don't want to be typed?

  • If required, are you ok with adding typing_extensions as an additional requirement?
    Should it be avoided at all costs?

  • If a situation arises where mypy errors unless a substantial refactor occurs, what is the desired solution?

    If it's not deemed possible to resolve the situation with type hints or variable annotations alone. Should the error be silenced, # type: ignore, and a todo, # TODO: ..., be added?

@cs01
Copy link
Contributor

cs01 commented Jan 29, 2020

I am not a maintainer but I added the mypy checks and type hints in #230, and expanded coverage in a few subsequent PRs.

Yes, they would like more type coverage. I don’t think there is anything that shouldn’t be typed. I know there is push for not adding new end-user dependencies to this project, so if you need that package, it would be preferable to only install it in the nox lint session where mypy is run (if that’s possible).

I have also run into cases where a substantial refactor is needed. In my experience, the maintainers prefer you make a PR with just the minimal refactor (to keep the diff reviewable), then add types in another PR.

I have been adding “type: ignore” where tricky things have arisen. I think that’s fine going forward. If we ever want to go back and fix them we can do a search for “type: ignore”. Right now nothing is typed so typing 99% of the code with a few ignores is much better than keeping none of it typed.

@Peilonrayz
Copy link
Contributor Author

Ok, that all sounds reasonable. Adding typing_extensions to just the lint session should be possible, if not a handy # type:ignore should suffice.

I'll start work on this soon, but I don't think I'll have any PRs for at least a week.

@theacodes
Copy link
Collaborator

I couldn't have said it any better than @cs01. I'd love more type hints. :)

@Peilonrayz
Copy link
Contributor Author

With the merger of #282 100% of the code has been typed. I'm going to close this as further issues should probably be their own issues.

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

No branches or pull requests

3 participants