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

Dedicated class pages are missing type hints for properties because of autosummary .. autoproperty:: quirk #2346

Open
Eric-Arellano opened this issue Nov 19, 2024 · 0 comments

Comments

@Eric-Arellano
Copy link
Collaborator

Eric-Arellano commented Nov 19, 2024

Problem: missing type hints

With Sphinx's autosummary extension, we're using .. autoattribute:: for functions marked with @property, rather than .. autoproperty:::

https://github.com/Qiskit/qiskit-addon-obp/blob/2412cb29c9d314a5ad568acdd5be558f7053c0f2/docs/_templates/autosummary/class.rst?plain=1#L20

This messes up the return type for properties. For example, the return type is defined on this @property:

https://github.com/Qiskit/qiskit-addon-mpf/blob/191dc8162510719ca3b50e5a25e9a1453419629a/qiskit_addon_mpf/backends/tenpy_tebd/evolver.py#L59-L62

However, the Sphinx output—and thus our docs—are missing that return type:

### evolved\_time
<Attribute id="qiskit_addon_mpf.backends.tenpy_tebd.TEBDEvolver.evolved_time">
Returns the current evolution time.
</Attribute>

This is fixed if we use .. autoproperty::; Sphinx will correctly set the return type.

So, why not use .. autoproperty::?

This is specifically an issue when we use .. autosummary:: to document a class. Properties work correctly when you use .. autoclass::, such as for inline class documentation in a module page.

The issue is that our template tells .. autoclass:: to not document any members so that we can instead manually document them by calling .. autoattribute and .. automethod:

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}
   :no-members:
   :no-inherited-members:
   :no-special-members:
   :show-inheritance:

{% block attributes_summary %}
  {% if attributes %}
   .. rubric:: Attributes
    {% for item in attributes %}
   .. autoattribute:: {{ item }}
    {%- endfor %}
  {% endif %}
{% endblock -%}

{% block methods_summary %}
  {% set wanted_methods = (methods | reject('==', '__init__') | list) %}
  {% if wanted_methods %}
   .. rubric:: Methods
    {% for item in wanted_methods %}
   .. automethod:: {{ item }}
    {%- endfor %}
  {% endif %}
{% endblock %}

In our Autosummary template, we manually list out every member so that we can add the lines .. rubric:: Attributes and .. rubric:: Methods, which get converted into h2 headings. These headings give important hierarchy to class pages that we do not want to lose:

Image

So, we cannot simply use .. autoclass:: to list all the members for us because we would lose the Attributes and Methods headings.

Ideally, we could do something like this:

{% block attributes_summary %}
  {% if attributes %}
   .. rubric:: Attributes
    {% for item in attributes %}
       {% if item is property %}
   .. autoproperty:: {{ item}}
       {% else %}
   .. autoattribute:: {{ item }}
       {% endif %}
    {%- endfor %}
  {% endif %}
{% endblock -%}

However, that type of if statement will not work because attributes is simply a list of strings, with both normal attributes and properties, such as ['foo', 'bar', 'my_prop']. There is no way in the Jinja template to determine whether the string corresponds to an @property, and Sphinx fails if you try using any Python builtins like dir() or hasattr(). What we really would need is for Autosummary to inject a properties variable, but it's not available: https://www.sphinx-doc.org/en/master/usage/extensions/autosummary.html#customizing-templates.

Rejected workaround: manually list every class

This wouldn't be a problem if we had docs authors give up on autosummary and manually call .. autoclass::, .. autoproperty::, .. autoattribute::, and .. automethod::, along with adding .. rubric:: Attributes and .. rubric:: Methods where appropriate. However, that is not realistic to expect.

Proposed solution: our script

We will switch the APIs to use .. autoclass:: in their Autosummary template to document their members, rather than manually iterating through them. Their template will look like this:

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}
   :show-inheritance:

That will fix the property issue, but now we'll be missing the Attributes and Methods h2 headings! So, we will have our qiskit/documentation script add back those headings to our MDX files in the appropriate location.

TBD:

  • Decide whether we should add the Attributes and Methods headings for inline classes on module pages.
    • Downside: increase the heading hierarchy for those classes.
    • Upside: more clear what is a method vs. attribute/property
@Eric-Arellano Eric-Arellano changed the title Autosummary template needs to use .. autoproperty:: for properties Dedicated class pages are missing type hints for properties because of autosummary .. autoproperty:: quirk Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

1 participant