From: Markus Armbruster <email@example.com> To: John Snow <firstname.lastname@example.org> Cc: Michael Roth <email@example.com>, Cleber Rosa <firstname.lastname@example.org>, email@example.com, Eduardo Habkost <firstname.lastname@example.org> Subject: Re: [PATCH v4 16/19] qapi/expr.py: Add docstrings Date: Wed, 21 Apr 2021 15:58:33 +0200 Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> (John Snow's message of "Tue, 20 Apr 2021 21:27:52 -0400") John Snow <email@example.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. """
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.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 \ email@example.com 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