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

[Markus Armbruster]

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.

Use your judgement.