python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
799 stars 111 forks source link

Make Python versions known statically #382

Closed layday closed 1 year ago

layday commented 1 year ago

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 https://github.com/python-attrs/cattrs/pull/379 (Pyright would merge the two branches).

codecov-commenter commented 1 year ago

Codecov Report

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

:exclamation: 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%) :arrow_down:
src/cattrs/gen/typeddicts.py 88.91% <100.00%> (+0.03%) :arrow_up:

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

Tinche commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thanks!