sphinxcontrib-django icon indicating copy to clipboard operation
sphinxcontrib-django copied to clipboard

Add missing type annotations and require them in new code

Open WhyNotHugo opened this issue 2 years ago • 10 comments

Short description

Add type annotations to all non-test code.

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

WhyNotHugo avatar Sep 27 '23 14:09 WhyNotHugo

I'd like to see if this fixes the issues mentioned in https://github.com/sphinx-doc/sphinxcontrib-django/pull/58#issuecomment-1736156596

WhyNotHugo avatar Sep 27 '23 14:09 WhyNotHugo

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (f4dfbbd) to head (cbba87e). Report is 6 commits behind head on main.

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 27 '23 14:09 codecov[bot]

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.

WhyNotHugo avatar Sep 27 '23 14:09 WhyNotHugo

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:

https://github.com/sphinx-doc/sphinxcontrib-django/blob/72705c6a7f714af4bfdd5e195332047549931935/sphinxcontrib_django/docstrings/patches.py#L90

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

timobrembeck avatar Sep 27 '23 14:09 timobrembeck

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?

WhyNotHugo avatar Oct 11 '23 15:10 WhyNotHugo

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? :thinking:

timobrembeck avatar Oct 11 '23 15:10 timobrembeck

@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 avatar Oct 11 '23 16:10 timobrembeck

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?)...

timobrembeck avatar Oct 11 '23 16:10 timobrembeck

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 avatar Oct 11 '23 18:10 WhyNotHugo

Apparently CI failed because codecov was down.

WhyNotHugo avatar Oct 11 '23 18:10 WhyNotHugo