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

Merged
merged 28 commits into from
Jun 19, 2024
Merged

Add type annotations #603

merged 28 commits into from
Jun 19, 2024

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Mar 26, 2024

This implements the bulk of the changes required for #558. The annotations are still a bit rough, but they have reached a stage where testing with third party code would be useful.

@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 missing coverage. Please review.

Project coverage is 89.98%. Comparing base (4275f84) to head (fb2fc5a).
Report is 3 commits behind head on master.

Current head fb2fc5a differs from pull request most recent head 9dc2247

Please upload reports for the commit 9dc2247 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.92%   89.98%   -8.94%     
==========================================
  Files           6        6              
  Lines         743      869     +126     
==========================================
+ Hits          735      782      +47     
- 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 mthuurne force-pushed the type-annotations branch 2 times, most recently from 4ee284d to aaa645b Compare May 7, 2024 08:42
@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 :)

@mthuurne mthuurne force-pushed the type-annotations branch 2 times, most recently from 8fe4407 to 0b5eec7 Compare June 12, 2024 15:45
@mthuurne
Copy link
Contributor Author

mthuurne commented Jun 12, 2024

This PR currently includes the commits from #614 and #624 as well. The history would be clearer if those were merged separately before merging this PR.

Other than that, I think this PR is ready for merging and beta testing.

@mthuurne mthuurne marked this pull request as ready for review June 12, 2024 16:04
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.
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.
The `JoinManager` class is deprecated and not annotated.
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.
In particular, `if TYPE_CHECKING:` blocks and `...` in bodies of
overloaded method definitions.
@mthuurne
Copy link
Contributor Author

I updated the coverage configuration to exclude lines that are only processed by mypy from the coverage measurement. However, rate limiting prevented the coverage upload from being accepted, so the shown coverage stats don't reflect the latest state.

@mthuurne
Copy link
Contributor Author

Actually, it's a bit weird that a project with as little activity as this one is running into rate limiting. Is it possible that the token we're using is old and doesn't specify that this is an open source project rather than a private evaluation?

@foarsitter
Copy link
Contributor

If the rate limiting is an issue we should open an dedicated issue to track it. I don't have the permissions to access the settings so I cannot be of any help.

Final confirmation: ready for merge?

@mthuurne
Copy link
Contributor Author

If the rate limiting is an issue we should open an dedicated issue to track it. I don't have the permissions to access the settings so I cannot be of any help.

Final confirmation: ready for merge?

Yes, it's ready.

The last change should make the coverage numbers more realistic by excluding lines that aren't expected to execute, but it doesn't change any functionality. So even if the change isn't correct, it won't negatively affect a preview release.

@foarsitter foarsitter merged commit 731ed80 into jazzband:master Jun 19, 2024
7 checks passed
@foarsitter
Copy link
Contributor

@mthuurne thanks for all the effort you have put into this! Great job!

@mthuurne mthuurne deleted the type-annotations branch June 19, 2024 20:04
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.

2 participants