QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
	Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v4 16/19] qapi/expr.py: Add docstrings
Date: Wed, 21 Apr 2021 15:58:33 +0200
Message-ID: <87y2db20za.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <6778db51-f0da-a4cd-079d-b577954acb75@redhat.com> (John Snow's message of "Tue, 20 Apr 2021 21:27:52 -0400")

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.
     """



  reply index

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25  6:03 [PATCH v4 00/19] qapi: static typing conversion, pt3 John Snow
2021-03-25  6:03 ` [PATCH v4 01/19] qapi/expr: Comment cleanup John Snow
2021-03-25 15:41   ` Markus Armbruster
2021-03-25 20:06     ` John Snow
2021-03-25  6:03 ` [PATCH v4 02/19] flake8: Enforce shorter line length for comments and docstrings John Snow
2021-03-25 15:21   ` Markus Armbruster
2021-03-25 20:20     ` John Snow
2021-03-26  6:26       ` Markus Armbruster
2021-03-26 16:30         ` John Snow
2021-03-26 16:44           ` Peter Maydell
2021-04-08  8:32             ` Markus Armbruster
2021-04-08  8:58             ` Daniel P. Berrangé
2021-04-09  9:33               ` Markus Armbruster
2021-04-09 17:08                 ` John Snow
2021-04-08  8:35           ` Markus Armbruster
2021-04-16 12:44   ` Markus Armbruster
2021-04-16 20:25     ` John Snow
2021-04-17 10:52       ` Markus Armbruster
2021-04-20 18:06         ` John Snow
2021-03-25  6:03 ` [PATCH v4 03/19] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
2021-03-25  6:03 ` [PATCH v4 04/19] qapi/expr.py: Check for dict instead of OrderedDict John Snow
2021-03-25  6:03 ` [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types John Snow
2021-03-25 14:04   ` Markus Armbruster
2021-03-25 20:48     ` John Snow
2021-03-26  5:40       ` Markus Armbruster
2021-03-26 17:12         ` John Snow
2021-03-25  6:03 ` [PATCH v4 06/19] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
2021-03-25  6:03 ` [PATCH v4 07/19] qapi/expr.py: move string check upwards in check_type John Snow
2021-03-25  6:03 ` [PATCH v4 08/19] qapi: add tests for invalid 'data' field type John Snow
2021-03-25 14:24   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 09/19] qapi/expr.py: Check type of 'data' member John Snow
2021-03-25 14:26   ` Markus Armbruster
2021-03-25 21:04     ` John Snow
2021-03-25  6:03 ` [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases John Snow
2021-03-25 14:33   ` Markus Armbruster
2021-03-25 23:32     ` John Snow
2021-03-25  6:03 ` [PATCH v4 11/19] qapi/expr.py: Modify check_keys to accept any Collection John Snow
2021-03-25 14:45   ` Markus Armbruster
2021-03-25 23:37     ` John Snow
2021-03-25  6:03 ` [PATCH v4 12/19] qapi/expr.py: add type hint annotations John Snow
2021-03-25  6:03 ` [PATCH v4 13/19] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
2021-03-25 15:15   ` Markus Armbruster
2021-03-26  0:07     ` John Snow
2021-03-25  6:03 ` [PATCH v4 14/19] qapi/expr.py: Remove single-letter variable John Snow
2021-03-25  6:03 ` [PATCH v4 15/19] qapi/expr.py: enable pylint checks John Snow
2021-03-25  6:03 ` [PATCH v4 16/19] qapi/expr.py: Add docstrings John Snow
2021-04-14 15:04   ` Markus Armbruster
2021-04-17  1:00     ` John Snow
2021-04-17 13:18       ` Markus Armbruster
2021-04-21  1:27         ` John Snow
2021-04-21 13:58           ` Markus Armbruster [this message]
2021-04-21 18:20             ` John Snow
2021-03-25  6:03 ` [PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for static data John Snow
2021-03-25 15:19   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 18/19] qapi/expr.py: move related checks inside check_xxx functions John Snow
2021-03-25  6:03 ` [PATCH v4 19/19] qapi/expr.py: Use an expression checker dispatch table John Snow
2021-03-25 15:46 ` [PATCH v4 00/19] qapi: static typing conversion, pt3 Markus Armbruster
2021-03-26  0:40 ` John Snow
2021-03-26 18:01 ` John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y2db20za.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=michael.roth@amd.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git