John Snow <js...@redhat.com> writes: > On 9/17/20 10:15 AM, Markus Armbruster wrote: >> John Snow <js...@redhat.com> writes: >> >>> Signed-off-by: John Snow <js...@redhat.com> >>> --- >>> scripts/qapi/common.py | 16 +++++++--------- >>> scripts/qapi/schema.py | 14 +++++++------- >>> 2 files changed, 14 insertions(+), 16 deletions(-) >>> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index 87d87b95e5..c665e67495 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -14,6 +14,11 @@ >>> import re >>> >>> +EATSPACE = '\033EATSPACE.' >>> +POINTER_SUFFIX = ' *' + EATSPACE >>> +c_name_trans = str.maketrans('.-', '__') >>> + >>> + >> You rename and move. pylint gripes about the names, but it doesn't >> actually ask for the move, as far as I can tell. Can you explain why >> you move? >> > > Preference. I like constants and globals at the top so you can audit > any code that runs at import time in one place.
I can buy this argument. > Since they are > externally visible objects, having them near other "header" style > information makes sense to me. This one I find unconvincing. Functions and classes are just as visible. Mention the move in the commit message, along with the (convincing part of the) rationale. Aside: EATSPACE and c_name_trans are actually local to common.py. EATSPACE sort of leaks out only via the contract of mcgen(). The contract could be rephrased in terms of POINTER_SUFFIX. Not sure it's worthwhile. >>> # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 >>> # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> >>> ENUM_NAME2 >>> # ENUM24_Name -> ENUM24_NAME >>> @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None): >>> return camel_to_upper(type_name) + '_' + c_name(const_name, >>> False).upper() >>> >>> -c_name_trans = str.maketrans('.-', '__') > > (This one winds up being a constant, so I renamed it in my v2.) > >>> - >>> - >>> # Map @name to a valid C identifier. >>> # If @protect, avoid returning certain ticklish identifiers (like >>> # C keywords) by prepending 'q_'. >>> @@ -89,10 +91,6 @@ def c_name(name, protect=True): >>> return name >>> >>> -eatspace = '\033EATSPACE.' >>> -pointer_suffix = ' *' + eatspace >>> - >>> - >>> class Indent: >>> """ >>> Indent-level management. >>> @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int: >>> >>> # Generate @code with @kwds interpolated. >>> -# Obey indent_level, and strip eatspace. >>> +# Obey INDENT level, and strip EATSPACE. >> Is the change to INDENT intentional? >> > > Kind of, but it's either late (should have been with the indent > manager patch) or early (Should be with the patch that moves comments > into docstrings.) > > When this comment becomes a docstring, I use `INDENT` to indicate it > as a proper object. This in and of itself is prescient, as we are not > using sphinx/apidoc to generate any documentation about the QAPI > package yet. > > (The pending v2 uses `indent` after you pointed out that it was not a > constant.) > >>> def cgen(code, **kwds): >>> raw = code % kwds >>> if INDENT: >>> raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, >>> flags=re.MULTILINE) >>> - return re.sub(re.escape(eatspace) + r' *', '', raw) >>> + return re.sub(re.escape(EATSPACE) + r' *', '', raw) >>> >>> def mcgen(code, **kwds): >> [...] >>