Re: [PATCH 14/37] qapi/common.py: Move comments into docstrings

[John Snow <jsnow@redhat.com>]

On 9/25/20 3:49 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 9/24/20 11:06 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 9/17/20 3:14 PM, Eduardo Habkost wrote:
>>>>> On Thu, Sep 17, 2020 at 02:44:53PM -0400, John Snow wrote:
>>>>> [...]
>>>>>> Having said this, I have not found any tool to date that actually *checks*
>>>>>> these comments for consistency. The pycharm IDE interactively highlights
>>>>>> them when it senses that you have made a mistake, but that cannot be worked
>>>>>> into our CI process that I know of - it's a proprietary checker.
>>>>>>
>>>>>> So right now, they're just plaintext that I am writing to approximate the
>>>>>> Sphinx style until such time as I enable autodoc for the module and
>>>>>> fine-tune the actual formatting and so on.
>>> You are deliberately trying to "approximate" Sphinx style, and ...
>>>
>>>>> After applying this series, I only had to make two small tweaks
>>>>> to make Sphinx + autodoc happy with the docstrings you wrote.
>>>>> With the following patch, "make sphinxdocs" will generate the
>>>>> QAPI Python module documentation at docs/devel/qapi.html.
>>>>> I had to explicitly skip qapi/doc.py because autodoc thinks the
>>>>> string constants are documentation strings.
>>>>>
>>>>
>>>> Awesome!
>>> ... actually almost nail it.
>>> Please mention your choice of style in the commit message.
>>>
>>
>> OK, I'll try to summarize it. I guess I'll do it in this commit
>> message, but it's not likely to be too visible in the future that way,
>> depending on how you run git blame and what you're looking at.
> 
> Thanks!
> 
>> I want to resurface my other python style patches soon; perhaps a
>> python coding style document should go in alongside those patches.
>>
>>>> I think I am going to delay explicitly pursuing writing a manual for
>>>> the QAPI parser for now, but it's good to know I am not too far
>>>> off. I'm going to target the mypy conversions first, because they can
>>>> be invasive and full of churn.
>>>>
>>>> When I get there, though ... I am thinking I should add this as
>>>> Devel/QAPI Parser.
>>> Doing "actually Sphinx style" instead of "an approximation of Sphinx
>>> style" would reduce churn later on.  So, if it's not too distracting...
>>>
>>
>> Yes, I just mean in terms of rebasing, docstrings and signatures fight
>> with each other for context and make re-spinning 125 patches a major
>> chore. I wasn't prepared to have the debate on if we wanted to add
>> Python code into the Sphinx generator and have that entire discussion
>> yet.
>>
>> So, I figured it would be a separate series afterwards. I mentioned
>> somewhere else that I anticipated doing it when I removed the
>> "missing-docstring" warning.
>>
>> I will investigate doing it (using Eduardo's patches) as a starting
>> point while reviews continue to filter in. If I figure it out in time,
>> I might squash the formatting changes in, but leave the sphinx
>> enablement patches themselves out.
> 

I wound up figuring it out before I sent V3 out :)

> Use your judgement.
> 

Yep. You can see how this played out in V3: I ensure that nothing is 
"wrong" but I haven't converted the entire submodule yet.

A thing to clarify for the list as well: Nothing validates these 
docstrings in the content-aware sense; nothing makes sure you documented 
the right parameter names, return values, exceptions, etc.

The only validation we can perform automatically right now that I am 
aware of is:

1. Did we have a docstring, yes/no?
2. Is anything referred to in `backticks` a valid reference?
3. Did that docstring parse as Sphinx-dialect ReST?

I would like to improve this situation someday, but it's not a task for 
the near future.

It would be very, very cool if autodoc itself was able to parse the 
integrity of the docstring and perform some simple validity checks, namely:

1. Every parameter has a :param FOO: in the docstring somewhere
2. Every :param FOO: matches a real parameter.
3. If the function has a return value, it is described with :returns:
4. If the function raises an exception, it must be mentioned with :raises:.

With perhaps some kind of exception for "simple" functions (How do I 
measure that?) where a one-line description is likely efficient enough.

--js