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 missing type annotations and require them in new code #61

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

WhyNotHugo
Copy link
Contributor

Short description

Add type annotations to all non-test code.

Also configure ruff to complain loudly if new code is missing annotations.

@WhyNotHugo
Copy link
Contributor Author

I'd like to see if this fixes the issues mentioned in #58 (comment)

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f4dfbbd) 100.00% compared to head (cbba87e) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #61   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          328       333    +5     
=========================================
+ Hits           328       333    +5     
Files Coverage Δ
sphinxcontrib_django/docstrings/attributes.py 100.00% <100.00%> (ø)
sphinxcontrib_django/docstrings/config.py 100.00% <100.00%> (ø)
sphinxcontrib_django/docstrings/data.py 100.00% <100.00%> (ø)
sphinxcontrib_django/docstrings/methods.py 100.00% <100.00%> (ø)
sphinxcontrib_django/docstrings/patches.py 100.00% <100.00%> (ø)

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

@WhyNotHugo
Copy link
Contributor Author

Building docs fails with:

/home/docs/checkouts/readthedocs.org/user_builds/sphinxcontrib-django/envs/61/lib/python3.11/site-packages/sphinxcontrib_django/docstrings/attributes.py:docstring of sphinxcontrib_django.docstrings.attributes.get_field_details:1: WARNING: py:class reference target not found: django.db.models.fields.Field

I think that this means that the class django.db.models.fields.Field isn't documented so a link can't be added to it. But I'm not sure if this is the actual cause of the failure.

@timobrembeck
Copy link
Collaborator

I think that this means that the class django.db.models.fields.Field isn't documented so a link can't be added to it. But I'm not sure if this is the actual cause of the failure.

Yes, the underlying issue is that Django documents the Field class as part of the django.db.models module and not django.db.models.fields:

https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field

This is also a part why this sphinx extension was created in the first place - it deals with such inconsistencies and monkeypatches the module path to its new location:

module_class.__module__ = parent_module_str

I'm not sure about this, but maybe the monkeypatch gets bypassed if the module is imported directly from its source?

This adds type hints everywhere except unit tests.
All the annotations here were added via `ruff check --fix .`.
@WhyNotHugo
Copy link
Contributor Author

I'm not sure about this, but maybe the monkeypatch gets bypassed if the module is imported directly from its source?

I don't think this code ever gets executed when type checking, right?

@timobrembeck
Copy link
Collaborator

I'm not sure about this, but maybe the monkeypatch gets bypassed if the module is imported directly from its source?

I don't think this code ever gets executed when type checking, right?

Indeed, but type checking is not the problem, the doc build is. And during doc build, the monkeypatch should be executed, but somehow it does not apply to the stuff that sphinx is extracting from the type hints? 🤔

@timobrembeck
Copy link
Collaborator

@WhyNotHugo And just because I could need this for another project:

Do you know a tool to automatically convert the :type my_argument: str docstrings to the def func(my_argument: str) syntax? Or did you do this manually in your PRs?
To me, it sounds like a task that could be automated, but I did not find a good tool for this except https://github.com/tox-dev/sphinx-autodoc-typehints which does not seem to be able to write the result back into the Python source files?

@timobrembeck
Copy link
Collaborator

Ah, but maybe can the extension sphinx_autodoc_typehints help to fix our problem here?
I mean it has the configuration option typehints_fully_qualified which is False by default (which is want we want, right?)...

@WhyNotHugo
Copy link
Contributor Author

Do you know a tool to automatically convert the :type my_argument: str docstrings to the def func(my_argument: str) syntax? Or did you do this manually in your PRs?

I did it manually. A tool that does this automatically would be superb.

To me, it sounds like a task that could be automated, but I did not find a good tool for this except https://github.com/tox-dev/sphinx-autodoc-typehints which does not seem to be able to write the result back into the Python source files?

I don't understand what this does. Sphinx already shows the types from type hints in the function signature by default. Example: https://django-afip.readthedocs.io/en/latest/api.html#django_afip.clients.get_client

@WhyNotHugo
Copy link
Contributor Author

Apparently CI failed because codecov was down.

@WhyNotHugo WhyNotHugo marked this pull request as ready for review October 11, 2023 18:37
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.

Ahh, I found the culprit:
The patches are not correctly applied, because during docbuild, only the sphinxcontrib_django.roles extension is enabled:

"sphinxcontrib_django.roles",

This means the patches are completely unused for our own docs. And there is no easy way to enable them without refactoring the structure of the extension, because the sphinxcontrib_django.docstrings extension needs Django settings and cannot be used without a standalone Django installation.
But I guess it makes sense to extract the patches from there, since patches are also relevant for Django library documentations, not only for Django applications.
I will refactor the structure of the extension first and then revisit this PR later!

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

2 participants