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

Make Python versions known statically #382

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

layday
Copy link
Contributor

@layday layday commented Jun 11, 2023

Type checkers are unable to interpret sys.version_info aliases and generate unions between Python version branches. If either branch depends on packages which are not installed the union might be reduced to an unknown or any type.

Follow-up from #379 (Pyright would merge the two branches).

Type checkers are unable to interpret `sys.version_info` aliases
and generate unions between Python version branches.  If either
branch depends on packages which are not installed the union
might be reduced to an unknown or any type.
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2023

Codecov Report

Merging #382 (88dffff) into main (b25a258) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
- Coverage   95.81%   95.80%   -0.02%     
==========================================
  Files          26       26              
  Lines        2128     2122       -6     
==========================================
- Hits         2039     2033       -6     
  Misses         89       89              
Impacted Files Coverage Δ
src/cattrs/_compat.py 95.92% <100.00%> (-0.13%) ⬇️
src/cattrs/gen/typeddicts.py 88.91% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Tinche
Copy link
Member

Tinche commented Jun 12, 2023

I'm willing to take this, but what's the benefit? I don't type check the insides of cattrs since it's very meta-programmy.

Will users of Pyright have a better time with these changes?

@layday
Copy link
Contributor Author

layday commented Jun 12, 2023

The benefit is that type checkers (Pyright and presumably MyPy) will be able to correctly resolve the types of some of cattrs' public classes (e.g. BaseValidationError and subclasses, the Mapping annotations of the Converter) which are tripping Pyright even after #379. The alternative would've been to e.g. use a typing-only base for these classes, but changing the version conditions seemed simple enough that I thought it might be preferable to concocting something specific to BaseValidationError and Mapping.

As an aside, even though the library's internals aren't worth type checking, public types could still be verified for correctness/completeness with mypy.stubtest or pyright --verifytypes.

@Tinche Tinche merged commit 18dff36 into python-attrs:main Jun 13, 2023
@Tinche
Copy link
Member

Tinche commented Jun 13, 2023

Thanks!

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.

3 participants