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 hints to docstrings module #46

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

WhyNotHugo
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Merging #46 (5ae6bbc) into main (872cb8d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #46   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          315       317    +2     
=========================================
+ Hits           315       317    +2     
Files Changed Coverage Δ
sphinxcontrib_django/docstrings/__init__.py 100.00% <100.00%> (ø)

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

Copy link
Collaborator

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WhyNotHugo Thanks a lot for your contribution! 💪

Could you exclude the if TYPE_CHECKING: from the code coverage report as described in nedbat/coveragepy#831? (Just in pyproject.toml instead of .coveragerc)

@WhyNotHugo
Copy link
Contributor Author

Updated. I've just realised that I've mis-measuring coverage in a bunch of project too, thanks for the pointer.

@WhyNotHugo
Copy link
Contributor Author

Rebased with pre-commit hooks applied.

- Add type hints
- Ignore type checking lines in coverage report

Co-authored-by: Timo Brembeck <github@timo.brembeck.email>
Copy link
Collaborator

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! 👍

I had to add the line pragma: no cover to the excluded lines as well, but now the coverage report looks good 👌

If you want to, you can also add type hints for the rest of the package 😁

Also, what do you think about including a tool like Mypy to check the types as part of the GitHub actions workflow?

@timobrembeck timobrembeck merged commit 09744e4 into sphinx-doc:main Sep 25, 2023
31 checks passed
@WhyNotHugo WhyNotHugo deleted the type-hints branch September 25, 2023 20:19
@WhyNotHugo
Copy link
Contributor Author

Mypy would help make sure that type hints keep making sense. 👍

Right now sphinxcontrib_django/docstrings/attributes.py is failing due to a quirk in the type system (tuple[X] and tuple[X, X] are different types):

sphinxcontrib_django/docstrings/attributes.py:20: error: Incompatible types in assignment (expression has type "tuple[Any, Any, Any]", variable has type "tuple[Any, Any]")  [assignment]

You might need to use:

FIELD_DESCRIPTORS : tuple[Any,...] = (FileDescriptor, related_descriptors.ForwardManyToOneDescriptor)

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.

None yet

3 participants