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

Add type annotations #603

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Conversation

mthuurne
Copy link
Contributor

This PR is only intended to share progress. It still needs work and once it is time to merge the changes should probably be split into smaller PRs for easier review.

Closes: #558

@mthuurne mthuurne marked this pull request as draft March 26, 2024 10:45
@mthuurne mthuurne changed the title Draft: Add type annotations Add type annotations Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 74.76038% with 79 lines in your changes are missing coverage. Please review.

Project coverage is 89.98%. Comparing base (512d0f1) to head (fb2fc5a).
Report is 6 commits behind head on master.

❗ Current head fb2fc5a differs from pull request most recent head aaa645b. Consider uploading reports for the commit aaa645b to get more accurate results

Files Patch % Lines
model_utils/managers.py 63.15% 49 Missing ⚠️
model_utils/choices.py 76.19% 10 Missing ⚠️
model_utils/models.py 64.00% 9 Missing ⚠️
model_utils/tracker.py 86.95% 9 Missing ⚠️
model_utils/fields.py 95.45% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
- Coverage   98.93%   89.98%   -8.95%     
==========================================
  Files           6        6              
  Lines         750      869     +119     
==========================================
+ Hits          742      782      +40     
- Misses          8       87      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mthuurne mthuurne force-pushed the type-annotations branch 2 times, most recently from e0bda54 to 87dfa2e Compare March 29, 2024 16:16
@mthuurne mthuurne force-pushed the type-annotations branch 4 times, most recently from a80bea3 to 98aa1ac Compare April 10, 2024 16:32
@mthuurne mthuurne force-pushed the type-annotations branch 4 times, most recently from 08a02e4 to d0b821a Compare April 16, 2024 13:03
@mthuurne mthuurne mentioned this pull request Apr 16, 2024
@mthuurne mthuurne force-pushed the type-annotations branch 2 times, most recently from 402da6c to cd2fcde Compare April 19, 2024 14:10
mthuurne added 12 commits May 6, 2024 12:38
This behavior wasn't documented and didn't fully work: it breaks
as soon as you try to save a model with a `None` value.
This allows using the latest annotation syntax supported by the type
checker regardless of the runtime Python version.
TODO: Review annotations after code changes made later.
The `Choices` constructor actually only accepts lists and tuples, not
arbitrary sequences. However, using `list` in the annotation opens
a can of worms related to type variance.
mthuurne added 17 commits May 7, 2024 10:41
This allows using the latest annotation syntax supported by the type
checker regardless of the runtime Python version.
These annotations are sufficient to pass mypy inspection if mypy is
configured to allow unannotated functions.
This required a bit of refactoring to get the type of `STATUS` correct
for each suite.

There are two cases which I decided not to support in the type system:
- passing a list instead of a tuple when defining an option group
- `in` checks using a data type that doesn't match the choices
We also type check the unit tests and the unit tests now import
functionality from pytest.
I can't find a way to inform mypy of the actual types without
duplicating a lot of test code.
This preserves the type of the wrapped descriptor (usually a field).

Maybe this is overkill, as `DescriptorWrapper` seems to only be used
as part of the `FieldTracker` implementation and is not documented
and barely tested. But technically, it is public API.
@foarsitter
Copy link
Contributor

@mthuurne time to merge this and release it as beta?

@mthuurne
Copy link
Contributor Author

@mthuurne time to merge this and release it as beta?

Ah sorry, I've been pretty busy lately organizing a conference (shameless plug: T-DOSE) and this escaped my attention.

I agree that doing a beta is a good idea. However, please let me do one more pass, to at least remove the "WIP:" prefixes from the commit messages.

@foarsitter
Copy link
Contributor

Sure @mthuurne, take your time :)

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.

Add type annotations
2 participants