| Message ID | 20200915224027.2529813-15-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
John Snow <jsnow@redhat.com> writes: > As docstrings, they'll show up in documentation and IDE help. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/common.py | 50 ++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index af01348b35..38d380f0a9 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -20,10 +20,18 @@ > c_name_trans = str.maketrans('.-', '__') > > > -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 > -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 > -# ENUM24_Name -> ENUM24_NAME > def camel_to_upper(value: str) -> str: > + """ > + Converts CamelCase to CAMEL_CASE. > + > + Examples: > + ENUMName -> ENUM_NAME > + EnumName1 -> ENUM_NAME1 > + ENUM_NAME -> ENUM_NAME > + ENUM_NAME1 -> ENUM_NAME1 > + ENUM_Name2 -> ENUM_NAME2 > + ENUM24_Name -> ENUM24_NAME > + """ > c_fun_str = c_name(value, False) > if value.isupper(): > return c_fun_str > @@ -45,21 +53,33 @@ def camel_to_upper(value: str) -> str: > def c_enum_const(type_name: str, > const_name: str, > prefix: Optional[str] = None) -> str: > + """ > + Generate a C enumeration constant name. > + > + :param type_name: The name of the enumeration. > + :param const_name: The name of this constant. > + :param prefix: Optional, prefix that overrides the type_name. > + """ Not actually a move. Suggest to retitle qapi/common: Turn comments in docstrings, and add more > if prefix is not None: > type_name = prefix > return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() > > > -# Map @name to a valid C identifier. > -# If @protect, avoid returning certain ticklish identifiers (like > -# C keywords) by prepending 'q_'. > -# > -# Used for converting 'name' from a 'name':'type' qapi definition > -# into a generated struct member, as well as converting type names > -# into substrings of a generated C function name. > -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' > -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' > def c_name(name: str, protect: bool = True) -> str: > + """ > + Map `name` to a valid C identifier. > + > + Used for converting 'name' from a 'name':'type' qapi definition > + into a generated struct member, as well as converting type names > + into substrings of a generated C function name. > + > + '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' > + protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' > + > + :param name: The name to map. > + :param protect: If true, avoid returning certain ticklish identifiers > + (like C keywords) by prepending 'q_'. > + """ > # ANSI X3J11/88-090, 3.1.1 > c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', > 'default', 'do', 'double', 'else', 'enum', 'extern', > @@ -135,9 +155,11 @@ def pop(self, amount: int = 4) -> int: > INDENT = Indent(0) > > > -# Generate @code with @kwds interpolated. > -# Obey INDENT level, and strip EATSPACE. > def cgen(code: str, **kwds: Union[str, int]) -> str: > + """ > + Generate `code` with `kwds` interpolated. > + Obey `INDENT`, and strip `EATSPACE`. > + """ > raw = code % kwds > if INDENT: > raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) Can you point to documentation on the docstring conventions and markup to use?
On 9/17/20 10:37 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> As docstrings, they'll show up in documentation and IDE help. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/common.py | 50 ++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 14 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index af01348b35..38d380f0a9 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -20,10 +20,18 @@ >> c_name_trans = str.maketrans('.-', '__') >> >> >> -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 >> -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 >> -# ENUM24_Name -> ENUM24_NAME >> def camel_to_upper(value: str) -> str: >> + """ >> + Converts CamelCase to CAMEL_CASE. >> + >> + Examples: >> + ENUMName -> ENUM_NAME >> + EnumName1 -> ENUM_NAME1 >> + ENUM_NAME -> ENUM_NAME >> + ENUM_NAME1 -> ENUM_NAME1 >> + ENUM_Name2 -> ENUM_NAME2 >> + ENUM24_Name -> ENUM24_NAME >> + """ >> c_fun_str = c_name(value, False) >> if value.isupper(): >> return c_fun_str >> @@ -45,21 +53,33 @@ def camel_to_upper(value: str) -> str: >> def c_enum_const(type_name: str, >> const_name: str, >> prefix: Optional[str] = None) -> str: >> + """ >> + Generate a C enumeration constant name. >> + >> + :param type_name: The name of the enumeration. >> + :param const_name: The name of this constant. >> + :param prefix: Optional, prefix that overrides the type_name. >> + """ > > Not actually a move. Suggest to retitle > > qapi/common: Turn comments in docstrings, and add more > OK. >> if prefix is not None: >> type_name = prefix >> return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() >> >> >> -# Map @name to a valid C identifier. >> -# If @protect, avoid returning certain ticklish identifiers (like >> -# C keywords) by prepending 'q_'. >> -# >> -# Used for converting 'name' from a 'name':'type' qapi definition >> -# into a generated struct member, as well as converting type names >> -# into substrings of a generated C function name. >> -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' >> -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >> def c_name(name: str, protect: bool = True) -> str: >> + """ >> + Map `name` to a valid C identifier. >> + >> + Used for converting 'name' from a 'name':'type' qapi definition >> + into a generated struct member, as well as converting type names >> + into substrings of a generated C function name. >> + >> + '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' >> + protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >> + >> + :param name: The name to map. >> + :param protect: If true, avoid returning certain ticklish identifiers >> + (like C keywords) by prepending 'q_'. >> + """ >> # ANSI X3J11/88-090, 3.1.1 >> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', >> 'default', 'do', 'double', 'else', 'enum', 'extern', >> @@ -135,9 +155,11 @@ def pop(self, amount: int = 4) -> int: >> INDENT = Indent(0) >> >> >> -# Generate @code with @kwds interpolated. >> -# Obey INDENT level, and strip EATSPACE. >> def cgen(code: str, **kwds: Union[str, int]) -> str: >> + """ >> + Generate `code` with `kwds` interpolated. >> + Obey `INDENT`, and strip `EATSPACE`. >> + """ >> raw = code % kwds >> if INDENT: >> raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) > > Can you point to documentation on the docstring conventions and markup > to use? > Short answer: No. Long answer: It's actually completely arbitrary, with major competing de-facto standards. Their primary function is to be stored to the __doc__ attribute of a module/class/method and can be displayed when using the interactive function help(...). https://www.python.org/dev/peps/pep-0257/ covers docstrings only in an extremely broad sense. In summary, it asks: - Use full sentences that end in periods - Use the triple-double quote style - multi-line docstrings should have their closing quote on a line by itself - multi-line docstrings should use a one-sentence summary, a blank line, and then a more elaborate description. It recommends you document arguments, types, return values and types, exceptions and so on but does not dictate a format. Two popular conventions are the google-style [1] and the NumPy-style [2] docstring formats. I write docstrings assuming we will be using *Sphinx* as our documentation tool. Sphinx does not read docstrings at all by default, but *autodoc* does. Autodoc assumes your docstrings are written in the Sphinx dialect of ReST. What you really want to look for is the "Python domain" documentation in Sphinx: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html Autodoc will annotate function/method docstrings with "py:function" or "py:method" as appropriate, but the actual contents of the block are still up to you. For those, you want to look up the Python domain info field lists that are supported by Sphinx, which are here: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists Taken together with PEP 257, you generally want something like this: """ Function f converts uncertainty into happiness. Function f only works on days that do not end in 'y'. Caution should be used when integrating this function into threaded code. :param uncertainty: All of your doubts. :raise RuntimeError: When it is a day ending in 'y'. :return: Your newfound happiness. """ I use the single-backtick as the Sphinx-ese "default role" resolver, which generally should resolve to a reference to some Python entity. The double-backtick is used to do formatted text for things like string literals and so on. Coffee break. 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. --js [1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html [2] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html#example-numpy
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. 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. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- docs/conf.py | 5 +++- docs/devel/index.rst | 1 + docs/devel/qapi.rst | 67 ++++++++++++++++++++++++++++++++++++++++++ scripts/qapi/common.py | 2 +- scripts/qapi/gen.py | 2 +- 5 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 docs/devel/qapi.rst diff --git a/docs/conf.py b/docs/conf.py index 8aeac40124..85be0e1860 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -54,6 +54,9 @@ except NameError: # sys.path.insert(0, os.path.join(qemu_docdir, "sphinx")) +# Make scripts/qapi module available for autodoc +sys.path.insert(0, os.path.join(qemu_docdir, "../scripts")) + # -- General configuration ------------------------------------------------ @@ -67,7 +70,7 @@ needs_sphinx = '1.6' # Add any Sphinx extension module names here, as strings. They can be # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom # ones. -extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile'] +extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile', 'sphinx.ext.autodoc'] # Add any paths that contain templates here, relative to this directory. templates_path = ['_templates'] diff --git a/docs/devel/index.rst b/docs/devel/index.rst index 04773ce076..a4d2cb9893 100644 --- a/docs/devel/index.rst +++ b/docs/devel/index.rst @@ -31,3 +31,4 @@ Contents: reset s390-dasd-ipl clocks + qapi diff --git a/docs/devel/qapi.rst b/docs/devel/qapi.rst new file mode 100644 index 0000000000..9130ef84c6 --- /dev/null +++ b/docs/devel/qapi.rst @@ -0,0 +1,67 @@ +QAPI Python module reference +============================ + +.. automodule:: qapi + :members: + :undoc-members: + +.. automodule:: qapi.commands + :members: + :undoc-members: + +.. automodule:: qapi.common + :members: + :undoc-members: + +.. automodule:: qapi.debug + :members: + :undoc-members: + +.. automodule:: qapi.error + :members: + :undoc-members: + +.. automodule:: qapi.events + :members: + :undoc-members: + +.. automodule:: qapi.expr + :members: + :undoc-members: + +.. automodule:: qapi.gen + :members: + :undoc-members: + +.. automodule:: qapi.introspect + :members: + :undoc-members: + +.. automodule:: qapi.params + :members: + :undoc-members: + +.. automodule:: qapi.parser + :members: + :undoc-members: + +.. automodule:: qapi.schema + :members: + :undoc-members: + +.. automodule:: qapi.script + :members: + :undoc-members: + +.. automodule:: qapi.source + :members: + :undoc-members: + +.. automodule:: qapi.types + :members: + :undoc-members: + +.. automodule:: qapi.visit + :members: + :undoc-members: + diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 0b1af694e6..7c8c4cb846 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -78,7 +78,7 @@ def c_name(name: str, protect: bool = True) -> str: :param name: The name to map. :param protect: If true, avoid returning certain ticklish identifiers - (like C keywords) by prepending 'q_'. + (like C keywords) by prepending ``q_``. """ # ANSI X3J11/88-090, 3.1.1 c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 0a72aecdd0..cf33732256 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -161,7 +161,7 @@ def ifcontext(ifcond: List[str], *args: QAPIGenCCode) -> Generator[None, None, None]: """A 'with' statement context manager to wrap with start_if()/end_if() - *args: any number of QAPIGenCCode + :param args: any number of QAPIGenCCode Example:: -- 2.26.2 -- Eduardo
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. > > 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! 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. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > docs/conf.py | 5 +++- > docs/devel/index.rst | 1 + > docs/devel/qapi.rst | 67 ++++++++++++++++++++++++++++++++++++++++++ > scripts/qapi/common.py | 2 +- > scripts/qapi/gen.py | 2 +- > 5 files changed, 74 insertions(+), 3 deletions(-) > create mode 100644 docs/devel/qapi.rst > > diff --git a/docs/conf.py b/docs/conf.py > index 8aeac40124..85be0e1860 100644 > --- a/docs/conf.py > +++ b/docs/conf.py > @@ -54,6 +54,9 @@ except NameError: > # > sys.path.insert(0, os.path.join(qemu_docdir, "sphinx")) > > +# Make scripts/qapi module available for autodoc > +sys.path.insert(0, os.path.join(qemu_docdir, "../scripts")) > + > > # -- General configuration ------------------------------------------------ > > @@ -67,7 +70,7 @@ needs_sphinx = '1.6' > # Add any Sphinx extension module names here, as strings. They can be > # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom > # ones. > -extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile'] > +extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile', 'sphinx.ext.autodoc'] > > # Add any paths that contain templates here, relative to this directory. > templates_path = ['_templates'] > diff --git a/docs/devel/index.rst b/docs/devel/index.rst > index 04773ce076..a4d2cb9893 100644 > --- a/docs/devel/index.rst > +++ b/docs/devel/index.rst > @@ -31,3 +31,4 @@ Contents: > reset > s390-dasd-ipl > clocks > + qapi > diff --git a/docs/devel/qapi.rst b/docs/devel/qapi.rst > new file mode 100644 > index 0000000000..9130ef84c6 > --- /dev/null > +++ b/docs/devel/qapi.rst > @@ -0,0 +1,67 @@ > +QAPI Python module reference > +============================ > + > +.. automodule:: qapi > + :members: > + :undoc-members: > + > +.. automodule:: qapi.commands > + :members: > + :undoc-members: > + > +.. automodule:: qapi.common > + :members: > + :undoc-members: > + > +.. automodule:: qapi.debug > + :members: > + :undoc-members: > + > +.. automodule:: qapi.error > + :members: > + :undoc-members: > + > +.. automodule:: qapi.events > + :members: > + :undoc-members: > + > +.. automodule:: qapi.expr > + :members: > + :undoc-members: > + > +.. automodule:: qapi.gen > + :members: > + :undoc-members: > + > +.. automodule:: qapi.introspect > + :members: > + :undoc-members: > + > +.. automodule:: qapi.params > + :members: > + :undoc-members: > + > +.. automodule:: qapi.parser > + :members: > + :undoc-members: > + > +.. automodule:: qapi.schema > + :members: > + :undoc-members: > + > +.. automodule:: qapi.script > + :members: > + :undoc-members: > + > +.. automodule:: qapi.source > + :members: > + :undoc-members: > + > +.. automodule:: qapi.types > + :members: > + :undoc-members: > + > +.. automodule:: qapi.visit > + :members: > + :undoc-members: > + > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index 0b1af694e6..7c8c4cb846 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -78,7 +78,7 @@ def c_name(name: str, protect: bool = True) -> str: > > :param name: The name to map. > :param protect: If true, avoid returning certain ticklish identifiers > - (like C keywords) by prepending 'q_'. > + (like C keywords) by prepending ``q_``. > """ > # ANSI X3J11/88-090, 3.1.1 > c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index 0a72aecdd0..cf33732256 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -161,7 +161,7 @@ def ifcontext(ifcond: List[str], > *args: QAPIGenCCode) -> Generator[None, None, None]: > """A 'with' statement context manager to wrap with start_if()/end_if() > > - *args: any number of QAPIGenCCode > + :param args: any number of QAPIGenCCode > > Example:: > >
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. > 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...
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. 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. --js
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.
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
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index af01348b35..38d380f0a9 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -20,10 +20,18 @@ c_name_trans = str.maketrans('.-', '__') -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 -# ENUM24_Name -> ENUM24_NAME def camel_to_upper(value: str) -> str: + """ + Converts CamelCase to CAMEL_CASE. + + Examples: + ENUMName -> ENUM_NAME + EnumName1 -> ENUM_NAME1 + ENUM_NAME -> ENUM_NAME + ENUM_NAME1 -> ENUM_NAME1 + ENUM_Name2 -> ENUM_NAME2 + ENUM24_Name -> ENUM24_NAME + """ c_fun_str = c_name(value, False) if value.isupper(): return c_fun_str @@ -45,21 +53,33 @@ def camel_to_upper(value: str) -> str: def c_enum_const(type_name: str, const_name: str, prefix: Optional[str] = None) -> str: + """ + Generate a C enumeration constant name. + + :param type_name: The name of the enumeration. + :param const_name: The name of this constant. + :param prefix: Optional, prefix that overrides the type_name. + """ if prefix is not None: type_name = prefix return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() -# Map @name to a valid C identifier. -# If @protect, avoid returning certain ticklish identifiers (like -# C keywords) by prepending 'q_'. -# -# Used for converting 'name' from a 'name':'type' qapi definition -# into a generated struct member, as well as converting type names -# into substrings of a generated C function name. -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' def c_name(name: str, protect: bool = True) -> str: + """ + Map `name` to a valid C identifier. + + Used for converting 'name' from a 'name':'type' qapi definition + into a generated struct member, as well as converting type names + into substrings of a generated C function name. + + '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo' + protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' + + :param name: The name to map. + :param protect: If true, avoid returning certain ticklish identifiers + (like C keywords) by prepending 'q_'. + """ # ANSI X3J11/88-090, 3.1.1 c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', 'default', 'do', 'double', 'else', 'enum', 'extern', @@ -135,9 +155,11 @@ def pop(self, amount: int = 4) -> int: INDENT = Indent(0) -# Generate @code with @kwds interpolated. -# Obey INDENT level, and strip EATSPACE. def cgen(code: str, **kwds: Union[str, int]) -> str: + """ + Generate `code` with `kwds` interpolated. + Obey `INDENT`, and strip `EATSPACE`. + """ raw = code % kwds if INDENT: raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
As docstrings, they'll show up in documentation and IDE help. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/common.py | 50 ++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 14 deletions(-)