[pydoclint] Add support for Sphinx-style docstrings in the pydoclint rules.
#13286
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| E501 | 18 | 0 | 18 | 0 | 0 |
| NPY002 | 8 | 0 | 8 | 0 | 0 |
| C408 | 4 | 0 | 4 | 0 | 0 |
| DOC201 | 2 | 0 | 2 | 0 | 0 |
| E741 | 1 | 0 | 1 | 0 | 0 |
| PLW0127 | 1 | 0 | 1 | 0 | 0 |
| PLW0128 | 1 | 0 | 1 | 0 | 0 |
| F811 | 1 | 0 | 1 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
|
Wow thanks. This is great. There are a lot of new ecosystem reports. Some of the superset violations look somewhat suspicious:
Like why are all the names empty? |
|
It might just be that the docstyle is a bit too aggressive right now. Should this be considered a sphinx docstring? Before I try to figure it out from the code. Did you gate the new code style behind preview mode? I think we have to because this change greatly increases the scope of the rule and it's probably good to get some feedback before rolling it out to everyone |
|
All the |
augustelalande
force-pushed
the
sphinx
branch
from
45ab7bf
to
c790e03
Compare
|
Looking at the airflow result I'm unsure if the parsing is correct. Could you take a look at them? Why is
Why is
The other thing I'm unsure about is whether the blank line rule between parameters should be disabled by default. All the examples I found don't require blank lines between parameters. What do you think? |
It is over indented relative to the docstring as a whole, since it is indented within the
Sphinx doesn't have sections per se, so to make the sphinxs docstrings fit into the mold in use for google and numpy docstrings, I consider each Note: It's not possible to combine all the param entries into a single param sections, since as far as I know there is no obligation that params must follow each other, although in practice they usually would. |
That makes sense but it seems confusing to users because I wouldn't consider those sections and it seems to cause many false positives in other rules. I would have to think this through more carefully but would we emit different diagnostics if we designed this from scratch? |
|
Everything sphinx can parse successfully with default settings should be fine, no? And sphinx definitely doesn't require blank lines between Here is a docstring that uses a lot of sphinx features and which is parsed correctly by sphinx. class SAM(Optimizer):
"""Sharpness Aware Minimization
Implements the 'Sharpness Aware Minimization' (SAM) algorithm introducued in
`Sharpness Aware Minimization`_) and the adaptive variant of it introduced in `ASAM`_.
We can use inline math: :math:`\\alpha`. You don't need the double backslash if
the python docstring is a raw string (r-string).
Block math:
.. math::
(a + b)^2 = a^2 + 2ab + b^2
.. note::
Implementation based on: https://github.com/cybertronai/pytorch-lamb
.. seealso:: :class:`LAMB`
.. _Sharpness Aware Minimization:
https://arxiv.org/abs/2010.01412
.. _ASAM:
https://arxiv.org/abs/2102.11600
:param base_optimizer: Base optimizer for SAM. (If the explanation goes
over two lines, the second line needs to be indented.
:param rho: Neighborhood size.
:raises ValueError: if ``rho`` is negative.
:example:
Everything associated with the example section needs to be indented, so that Sphinx
knows what belongs to the example.
.. code-block:: python
# Use AdamW as the base optimizer.
base_optimizer = AdamW(model.parameters())
# Wrap the base optimizer in SAM.
optimizer = SAM(base_optimizer)
# Closure required for recomputing the loss after computing epsilon(w).
def _closure():
return loss_function(logits=model(input), targets=targets)
loss = _closure()
loss.backward()
optimizer.step(closure=_closure)
optimizer.zero_grad()
You can also write it in this syntax:
>>> import template
>>> a = template.MainClass1()
>>> a.function1(1,1,1)
2
""" |
We probably would not emit D410 among others for sphinx style docstrings. And in fact if the user specifies sphinx as their docstring convention then those rules get disabled (each convention runs a different subset of D). The problem is when the convention is unspecified we still run it. One solution would be to disable innapropriate rules if sphinx is detected. There is some precendence for that, and I have already done it for some of them. I don't know what a "proper" implementation of "D410" would look like for sphinx, since there are no "sections" per se |
|
One of the problems is that |
MichaReiser
added
rule
|
So, is the problem here that pydocstyle and pydoclint are incompatible? I see a |
|
One thing I'm not following from the conversation is why there are so many Another thing we could do is: never infer Sphinx. Require that users set it via a Finally... I think pydocstyle did add Sphinx support, at least for D417? Can we make sure that whatever we do here is consistent with the pydocstyle implementation? See: PyCQA/pydocstyle#595 |
|
Per above, I have improved the gating so sphinx will only be active in preview mode. I have also implemented the change to never infer sphinx, which should prevent a lot of the issues. But I am concerned that this prevents the ecosystem check to work properly, since they don't specify sphinx in their configs. Would love to hear some insights on this. |
|
Ok so D417 is undocumented-param, which I think I can address in #13280 if this gets merged first (otherwise I'll add it here). Note, we already don't have full parity with D417 since the ruff version only supports google style. |
Summary
Add support for Sphinx-style docstrings in the
pydoclintrules. This also defines (out of necessity) the set of pydocstyle rules enforced when the sphinx style is selected. For lack of a good reference I just copied the exclusions from the pep257 selection, minusRule::SectionNotOverIndented.Part of #12434.
Resolves #12520.
Test Plan
Test cases added.