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

Enable Mypy --strict internally #514

Closed
michaeloliverx opened this issue Mar 1, 2022 · 13 comments · Fixed by #616
Closed

Enable Mypy --strict internally #514

michaeloliverx opened this issue Mar 1, 2022 · 13 comments · Fixed by #616
Assignees

Comments

@michaeloliverx
Copy link
Member

michaeloliverx commented Mar 1, 2022

I think being compatible with mypy --strict benefits end users who are also using it, if #513 is merged then there isn't much work left to be compatible with strict mode.

See also #512

@michaeloliverx
Copy link
Member Author

What do @encode/maintainers think of this?

@adriangb
Copy link
Member

Would we do it only for the codebase or also for tests? The benefit of doing it for tests is that you get to asses what the experience would be like for a user in strict mode.

@adriangb
Copy link
Member

I see you did both in your PR. I'm in favor of it 😁

@tomchristie
Copy link
Member

Based on #524 I'd probably be somewhat against it - looks like extra noise & fluff from my perspective.

@michaeloliverx
Copy link
Member Author

Would we do it only for the codebase or also for tests? The benefit of doing it for tests is that you get to asses what the experience would be like for a user in strict mode.

+1 for tests

I didn't type tests before I read this article https://sethmlarson.dev/blog/tests-arent-enough-case-study-after-adding-types-to-urllib3#type-your-tests which outlined the same benefit.

@michaeloliverx
Copy link
Member Author

Based on #524 I'd probably be somewhat against it - looks like extra noise & fluff from my perspective.

I agree its quite a bit of noise.. another option would be to do it incrementally as files are changed?

@RA80533
Copy link

RA80533 commented Apr 24, 2022

Based on #524 I'd probably be somewhat against it - looks like extra noise & fluff from my perspective.

Are there particular changes in the PR that you would rather see addressed separately, @tomchristie?

  1. Narrowing # type: ignore to a particular class of error
  2. Correcting incomplete dict and list types
  3. Using type aliases on values with domain significance (HeadersAsMapping versus dict)
  4. Casting types brought in from the h2 package

2. is the most important for the purpose of the PR. Each correction means one less error showing up to the user in VS Code and the like.

@stale
Copy link

stale bot commented May 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 31, 2022
@stale stale bot closed this as completed Jun 19, 2022
@graingert graingert reopened this Jun 19, 2022
@graingert graingert removed the wontfix This will not be worked on label Jun 19, 2022
@graingert
Copy link
Member

adding typing in anyio found a large number of type level bugs, I'm in favor of this change - so unmarking stale

@tomchristie
Copy link
Member

I'm okay with us doing this, or at least with us moving towards it incrementally.
@michaeloliverx's work on #524 showed that it's quite a big change footprint.
I'd be happy to see PRs that either deal with changes on a one-module-at-a-time or one-kind-of-fix-at-a-time basis.

@adriangb
Copy link
Member

adriangb commented Oct 7, 2022

I think we probably want to turn on --strict one file at a time right?

@tomchristie
Copy link
Member

Yeah that sort of thing.
It'd probably be okay to make the fixes incrementally, without necessarily ratcheting them in using the CI.
We could improve them bit by bit and only hard-enforce it once we get there.

@tomchristie
Copy link
Member

See encode/httpx#2436 for how I think we should approach this.

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 a pull request may close this issue.

5 participants