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

fix: mypy update #606

Merged
merged 2 commits into from
Apr 24, 2023
Merged

fix: mypy update #606

merged 2 commits into from
Apr 24, 2023

Conversation

henryiii
Copy link
Contributor

This fixes the issue with two type ignores no longer being needed, updates mypy, moves the configuration out of extras (it was outdated and not helpful to force pin mypy in extras, and not required to use mypy to type anyway, and removes the requirement that build be installed to type check (which is not needed).

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@layday
Copy link
Member

layday commented Apr 23, 2023

I'm not a fan of specifying dependencies in tox.ini, personally. I wanna be able to pip install -e .[all] and be in a position to test, type check and lint the code. Like, how would I set this up with my editor? Do I need to run the type check session at least once and then use the venv tox creates specifically for type checking?

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Contributor Author

henryiii commented Apr 24, 2023

I've reverted the env changes for now, so the CI fix can go though I don't like the current setup:

  • mypy should not be required in a public extra - I'd expect build[typing] to install build + whatever it needs to be used in static typing, not a type checker.
  • You shouldn't have to install the package when type checking it.

The example you gave doesn't work today, we don't provide an all? And we can't without a lot of duplication or dynamic metadata for extras...

I'd normally put the type check in pre-commit for any smaller package, but the drawbacks are the same (though bumping mypy would be easier). Honestly, the best way to do this today that would support all possible ways to run it, like in editors, would probably be a requirements.txt. And I still don't know if you'd put mypy in it - several editors have their own type checkers.

I don't feel very strongly about it, though, so as long as we can keep things up to date and working, I'm fine if it can't be ideal IMO.

@henryiii henryiii changed the title fix: mypy update & env change fix: mypy update Apr 24, 2023
@layday layday merged commit f1aa873 into pypa:main Apr 24, 2023
55 checks passed
@layday
Copy link
Member

layday commented Apr 24, 2023

You shouldn't have to install the package when type checking it.

I'm not sure I agree. For development, probably not. But before publishing, I feel you should type check the built package, just as you would test against the built package.

And we can't without a lot of duplication or dynamic metadata for extras...

We can do with self-referencing extras. But [all] was just an example.

I'd normally put the type check in pre-commit [...]

Maybe I should write a tool to parse pre-commit dependencies and install them in the current env with pip, for troglodytes comme moi to use.

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

2 participants