Re: [PATCH v4 16/19] qapi/expr.py: Add docstrings

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

[...]
> I've made a re-spin. Let's try something new, if you don't mind:
>
> I've pushed a "almost v5" copy onto my gitlab, where edits made against 
> this patch are in their own commit so that all of the pending edits I've 
> made are easily visible.
>
> Here's the "merge request", which I made against my own fork of master:
> https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs
>
> (It's marked "WIP", so there's no risk of me accidentally merging it -- 
> and if I did, it would be to my own "master" branch, so no worries about 
> us goofing this up.)
>
> If you click "Commits (21)" at the top, underneath "WIP: 
> python-qapi-cleanup-pt3", you can see the list of commits in the re-spin.
>
> (Four of these commits are the DO-NOT-MERGE ones I carry around as a 
> testing pre-requisite.)
>
>  From here, you can see the "[RFC] docstring diff" patch which shows all 
> the edits I've made so far based on your feedback and my tinkering.
>
> https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs?commit_id=3f0e9fb71304edb381ce3b9bf0ff08624fb277bc
>
> I invite you to leave feedback here on this view (and anywhere else in 
> the series that still needs adjusting, if you are so willing to humor 
> me) by highlighting the line and clicking the comment box icon on the 
> left. If you left-click and drag the comment box, you can target a range 
> of lines.
>
> (You can even propose a diff directly using this method, which allows me 
> to just accept your proposal directly.)
>
> If you leave any comments here, I can resolve each individual nugget of 
> feedback by clicking "Resolve Thread" in my view, which will help me 
> keep track of which items I believe I have addressed and which items I 
> have not. This will help me make sure I don't miss any of your feedback, 
> and it helps me keep track of what edits I've made for the next changelog.
>
> Willing to try it out?
>
> Once we're both happy with it, I will send it back to the list for final 
> assessment using our traditional process. Anyone else who wants to come 
> comment on the gitlab draft is of course more than welcome to.

I have only a few minor remarks, and I'm too lazy to create a gitlab
account just for them.

* Commit 3f0e9fb713 qapi/expr: [RFC] docstring diff

  - You mixed up check_name_lower() and check_name_camel()

  - Nitpick: check_defn_name_str() has inconsistent function name
    markup.

  - I'd like to suggest a tweak of check_defn_name_str() :param meta:

  That's all.  Converged quickly.  Nice!  Incremental diff appended.

* Old "[PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for
  static data" is gone.  I think this leaves commit 913e3fd6f8's "Later
  patches will make use of that" dangling.  Let's not drop old PATCH 17.
  Put it right after 913e3fd6f8 if that's trivial.  If not, put it
  wherever it creates the least work for you.


diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f2bb92ab79..5c9060cb1b 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -124,7 +124,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str,
                      permit_upper: bool = False,
                      permit_underscore: bool = False) -> None:
     """
-    Ensure that ``name`` is a valid user defined type name.
+    Ensure that ``name`` is a valid command or member name.
 
     This means it must be a valid QAPI name as checked by
     `check_name_str()`, but where the stem prohibits uppercase
@@ -147,7 +147,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str,
 
 def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None:
     """
-    Ensure that ``name`` is a valid command or member name.
+    Ensure that ``name`` is a valid user-defined type name.
 
     This means it must be a valid QAPI name as checked by
     `check_name_str()`, but where the stem must be in CamelCase.
@@ -168,14 +168,14 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
     Ensure that ``name`` is a valid definition name.
 
     Based on the value of ``meta``, this means that:
-      - 'event' names adhere to `check_name_upper`.
-      - 'command' names adhere to `check_name_lower`.
+      - 'event' names adhere to `check_name_upper()`.
+      - 'command' names adhere to `check_name_lower()`.
       - Else, meta is a type, and must pass `check_name_camel()`.
         These names must not end with ``Kind`` nor ``List``.
 
     :param name: Name to check.
     :param info: QAPI schema source file information.
-    :param meta: Type name of the QAPI expression.
+    :param meta: Meta-type name of the QAPI expression.
 
     :raise QAPISemError: When ``name`` fails validation.
     """