Message ID | 20210323094025.3569441-11-armbru@redhat.com |
---|---|
State | New |
Headers | show |
Series | qapi: Enforce naming rules | expand |
On 3/23/21 4:40 AM, Markus Armbruster wrote: > Naming rules differ for the various kinds of names. To prepare > enforcing them, define functions to check them: check_name_upper(), > check_name_lower(), and check_name_camel(). For now, these merely > wrap around check_name_str(), but that will change shortly. Replace > the other uses of check_name_str() by appropriate uses of the > wrappers. No change in behavior just yet. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 15 deletions(-) > > +++ b/scripts/qapi/expr.py > @@ -21,11 +21,12 @@ > from .error import QAPISemError > > > -# Names must be letters, numbers, -, and _. They must start with letter, > -# except for downstream extensions which must start with __RFQDN_. > -# Dots are only valid in the downstream extension prefix. > -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' > - '[a-zA-Z][a-zA-Z0-9_-]*$') I'm assuming python concatenates r'' with '' in the obvious manner... > +# Names consist of letters, digits, -, and _, starting with a letter. > +# An experimental name is prefixed with x-. A name of a downstream > +# extension is prefixed with __RFQDN_. The latter prefix goes first. > +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' > + r'(x-)?' > + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) ...but like your explicit use of r'' r''. Splitting out special handling of r'(x-)?' does not change behavior, but is not otherwise mentioned in your commit message. I suspect you did it to make it easier to permit x-EVENT_NAME in later patches where upper is handled differently from lower or camel, so I won't withhold R-b, but it may be worth a tweak to the commit message. > > def check_defn_name_str(name, info, meta): > - check_name_str(name, info, meta, permit_upper=True) > + if meta == 'event': > + check_name_upper(name, info, meta) > + elif meta == 'command': > + check_name_lower(name, info, meta, permit_upper=True) Why do commands need to permit upper? I guess just downstream FQDN extensions? Otherwise the patch makes sense. Reviewed-by: Eric Blake <eblake@redhat.com>
On 3/23/21 10:20 AM, Eric Blake wrote: >> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >> - '[a-zA-Z][a-zA-Z0-9_-]*$') > I'm assuming python concatenates r'' with '' in the obvious manner... > FWIW, I don't think it does, actually. I believe you do need to spell out each individual string constant with what type it is. (In this case, missing the second r has no effect as there are no backslash sequences in the string.) --js
On 3/23/21 9:30 AM, John Snow wrote: > On 3/23/21 10:20 AM, Eric Blake wrote: >>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >> I'm assuming python concatenates r'' with '' in the obvious manner... >> > > FWIW, I don't think it does, actually. I believe you do need to spell > out each individual string constant with what type it is. > > (In this case, missing the second r has no effect as there are no > backslash sequences in the string.) Aha - https://docs.python.org/3/reference/lexical_analysis.html#string-literal-concatenation talks about it more, and even mentions that joining r'' with plain '' is useful for scenarios where you want easier use of \ through only part of your overall literal (since string literal concatenation is performed at compile time).
Eric Blake <eblake@redhat.com> writes: > On 3/23/21 4:40 AM, Markus Armbruster wrote: >> Naming rules differ for the various kinds of names. To prepare >> enforcing them, define functions to check them: check_name_upper(), >> check_name_lower(), and check_name_camel(). For now, these merely >> wrap around check_name_str(), but that will change shortly. Replace >> the other uses of check_name_str() by appropriate uses of the >> wrappers. No change in behavior just yet. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >> 1 file changed, 36 insertions(+), 15 deletions(-) >> > >> +++ b/scripts/qapi/expr.py >> @@ -21,11 +21,12 @@ >> from .error import QAPISemError >> >> >> -# Names must be letters, numbers, -, and _. They must start with letter, >> -# except for downstream extensions which must start with __RFQDN_. >> -# Dots are only valid in the downstream extension prefix. >> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >> - '[a-zA-Z][a-zA-Z0-9_-]*$') > > I'm assuming python concatenates r'' with '' in the obvious manner... > >> +# Names consist of letters, digits, -, and _, starting with a letter. >> +# An experimental name is prefixed with x-. A name of a downstream >> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >> + r'(x-)?' >> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) > > ...but like your explicit use of r'' r''. > > Splitting out special handling of r'(x-)?' does not change behavior, but > is not otherwise mentioned in your commit message. I suspect you did it > to make it easier to permit x-EVENT_NAME in later patches where upper is > handled differently from lower or camel, Yes. > so I won't withhold R-b, but it > may be worth a tweak to the commit message. Probably. I'm failing at coming up with a concise text that isn't confusing. >> def check_defn_name_str(name, info, meta): >> - check_name_str(name, info, meta, permit_upper=True) >> + if meta == 'event': >> + check_name_upper(name, info, meta) >> + elif meta == 'command': >> + check_name_lower(name, info, meta, permit_upper=True) > > Why do commands need to permit upper? I guess just downstream FQDN > extensions? This is just so that the patch doesn't change behavior. PATCH 24 will flip it to False. > Otherwise the patch makes sense. > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 3/23/21 4:40 AM, Markus Armbruster wrote: >>> Naming rules differ for the various kinds of names. To prepare >>> enforcing them, define functions to check them: check_name_upper(), >>> check_name_lower(), and check_name_camel(). For now, these merely >>> wrap around check_name_str(), but that will change shortly. Replace >>> the other uses of check_name_str() by appropriate uses of the >>> wrappers. No change in behavior just yet. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 36 insertions(+), 15 deletions(-) >>> >> >>> +++ b/scripts/qapi/expr.py >>> @@ -21,11 +21,12 @@ >>> from .error import QAPISemError >>> >>> >>> -# Names must be letters, numbers, -, and _. They must start with letter, >>> -# except for downstream extensions which must start with __RFQDN_. >>> -# Dots are only valid in the downstream extension prefix. >>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >> >> I'm assuming python concatenates r'' with '' in the obvious manner... >> >>> +# Names consist of letters, digits, -, and _, starting with a letter. >>> +# An experimental name is prefixed with x-. A name of a downstream >>> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >>> + r'(x-)?' >>> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >> >> ...but like your explicit use of r'' r''. >> >> Splitting out special handling of r'(x-)?' does not change behavior, but >> is not otherwise mentioned in your commit message. I suspect you did it >> to make it easier to permit x-EVENT_NAME in later patches where upper is >> handled differently from lower or camel, > > Yes. > >> so I won't withhold R-b, but it >> may be worth a tweak to the commit message. > > Probably. I'm failing at coming up with a concise text that isn't > confusing. Adding this paragraph: check_name_str() now returns the name without downstream and x- prefix, for use by the wrappers in later patches. Requires tweaking regexp @valid_name. It accepts the same strings as before.
On 3/23/21 5:40 AM, Markus Armbruster wrote: > Naming rules differ for the various kinds of names. To prepare > enforcing them, define functions to check them: check_name_upper(), > check_name_lower(), and check_name_camel(). For now, these merely > wrap around check_name_str(), but that will change shortly. Replace > the other uses of check_name_str() by appropriate uses of the > wrappers. No change in behavior just yet. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 15 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index e00467636c..30285fe334 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -21,11 +21,12 @@ > from .error import QAPISemError > > > -# Names must be letters, numbers, -, and _. They must start with letter, > -# except for downstream extensions which must start with __RFQDN_. > -# Dots are only valid in the downstream extension prefix. > -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' > - '[a-zA-Z][a-zA-Z0-9_-]*$') > +# Names consist of letters, digits, -, and _, starting with a letter. > +# An experimental name is prefixed with x-. A name of a downstream > +# extension is prefixed with __RFQDN_. The latter prefix goes first. > +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' > + r'(x-)?' > + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) > > > def check_name_is_str(name, info, source): > @@ -37,16 +38,38 @@ def check_name_str(name, info, source, > permit_upper=False): > # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' > # and 'q_obj_*' implicit type names. > - if not valid_name.match(name) or \ > - c_name(name, False).startswith('q_'): > + match = valid_name.match(name) > + if not match or c_name(name, False).startswith('q_'): > raise QAPISemError(info, "%s has an invalid name" % source) > if not permit_upper and name.lower() != name: > raise QAPISemError( > info, "%s uses uppercase in name" % source) > + return match.group(3) > + > + > +def check_name_upper(name, info, source): > + stem = check_name_str(name, info, source, permit_upper=True) > + # TODO reject '[a-z-]' in @stem > + Creates (presumably) temporary errors in flake8 for the dead assignment here and below. > + > +def check_name_lower(name, info, source, > + permit_upper=False): > + stem = check_name_str(name, info, source, permit_upper) > + # TODO reject '_' in stem > + > + > +def check_name_camel(name, info, source): > + stem = check_name_str(name, info, source, permit_upper=True) > + # TODO reject '[_-]' in stem, require CamelCase > > > def check_defn_name_str(name, info, meta): > - check_name_str(name, info, meta, permit_upper=True) > + if meta == 'event': > + check_name_upper(name, info, meta) > + elif meta == 'command': > + check_name_lower(name, info, meta, permit_upper=True) > + else: > + check_name_camel(name, info, meta) > if name.endswith('Kind') or name.endswith('List'): > raise QAPISemError( > info, "%s name should not end in '%s'" % (meta, name[-4:])) > @@ -163,8 +186,7 @@ def check_type(value, info, source, > key_source = "%s member '%s'" % (source, key) > if key.startswith('*'): > key = key[1:] > - check_name_str(key, info, key_source, > - permit_upper=permit_upper) > + check_name_lower(key, info, key_source, permit_upper) > if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): > raise QAPISemError(info, "%s uses reserved name" % key_source) > check_keys(arg, info, key_source, ['type'], ['if', 'features']) > @@ -186,7 +208,7 @@ def check_features(features, info): > check_keys(f, info, source, ['name'], ['if']) > check_name_is_str(f['name'], info, source) > source = "%s '%s'" % (source, f['name']) > - check_name_str(f['name'], info, source) > + check_name_lower(f['name'], info, source) > check_if(f, info, source) > > > @@ -213,8 +235,7 @@ def check_enum(expr, info): > # Enum members may start with a digit > if member_name[0].isdigit(): > member_name = 'd' + member_name # Hack: hide the digit > - check_name_str(member_name, info, source, > - permit_upper=permit_upper) > + check_name_lower(member_name, info, source, permit_upper) > check_if(member, info, source) > > > @@ -244,7 +265,7 @@ def check_union(expr, info): > for (key, value) in members.items(): > source = "'data' member '%s'" % key > if discriminator is None: > - check_name_str(key, info, source) > + check_name_lower(key, info, source) > # else: name is in discriminator enum, which gets checked > check_keys(value, info, source, ['type'], ['if']) > check_if(value, info, source) > @@ -258,7 +279,7 @@ def check_alternate(expr, info): > raise QAPISemError(info, "'data' must not be empty") > for (key, value) in members.items(): > source = "'data' member '%s'" % key > - check_name_str(key, info, source) > + check_name_lower(key, info, source) > check_keys(value, info, source, ['type'], ['if']) > check_if(value, info, source) > check_type(value['type'], info, source) >
John Snow <jsnow@redhat.com> writes: > On 3/23/21 5:40 AM, Markus Armbruster wrote: >> Naming rules differ for the various kinds of names. To prepare >> enforcing them, define functions to check them: check_name_upper(), >> check_name_lower(), and check_name_camel(). For now, these merely >> wrap around check_name_str(), but that will change shortly. Replace >> the other uses of check_name_str() by appropriate uses of the >> wrappers. No change in behavior just yet. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >> 1 file changed, 36 insertions(+), 15 deletions(-) >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index e00467636c..30285fe334 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -21,11 +21,12 @@ >> from .error import QAPISemError >> >> -# Names must be letters, numbers, -, and _. They must start with letter, >> -# except for downstream extensions which must start with __RFQDN_. >> -# Dots are only valid in the downstream extension prefix. >> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >> - '[a-zA-Z][a-zA-Z0-9_-]*$') >> +# Names consist of letters, digits, -, and _, starting with a letter. >> +# An experimental name is prefixed with x-. A name of a downstream >> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >> + r'(x-)?' >> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >> >> def check_name_is_str(name, info, source): >> @@ -37,16 +38,38 @@ def check_name_str(name, info, source, >> permit_upper=False): >> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >> # and 'q_obj_*' implicit type names. >> - if not valid_name.match(name) or \ >> - c_name(name, False).startswith('q_'): >> + match = valid_name.match(name) >> + if not match or c_name(name, False).startswith('q_'): >> raise QAPISemError(info, "%s has an invalid name" % source) >> if not permit_upper and name.lower() != name: >> raise QAPISemError( >> info, "%s uses uppercase in name" % source) >> + return match.group(3) >> + >> + >> +def check_name_upper(name, info, source): >> + stem = check_name_str(name, info, source, permit_upper=True) >> + # TODO reject '[a-z-]' in @stem >> + > > Creates (presumably) temporary errors in flake8 for the dead > assignment here and below. All gone by the end of the series. "make check" and checkpatch were content. Anything else you'd like me to run?
On 3/24/21 1:57 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 3/23/21 5:40 AM, Markus Armbruster wrote: >>> Naming rules differ for the various kinds of names. To prepare >>> enforcing them, define functions to check them: check_name_upper(), >>> check_name_lower(), and check_name_camel(). For now, these merely >>> wrap around check_name_str(), but that will change shortly. Replace >>> the other uses of check_name_str() by appropriate uses of the >>> wrappers. No change in behavior just yet. >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 36 insertions(+), 15 deletions(-) >>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>> index e00467636c..30285fe334 100644 >>> --- a/scripts/qapi/expr.py >>> +++ b/scripts/qapi/expr.py >>> @@ -21,11 +21,12 @@ >>> from .error import QAPISemError >>> >>> -# Names must be letters, numbers, -, and _. They must start with letter, >>> -# except for downstream extensions which must start with __RFQDN_. >>> -# Dots are only valid in the downstream extension prefix. >>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >>> +# Names consist of letters, digits, -, and _, starting with a letter. >>> +# An experimental name is prefixed with x-. A name of a downstream >>> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >>> + r'(x-)?' >>> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >>> >>> def check_name_is_str(name, info, source): >>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source, >>> permit_upper=False): >>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >>> # and 'q_obj_*' implicit type names. >>> - if not valid_name.match(name) or \ >>> - c_name(name, False).startswith('q_'): >>> + match = valid_name.match(name) >>> + if not match or c_name(name, False).startswith('q_'): >>> raise QAPISemError(info, "%s has an invalid name" % source) >>> if not permit_upper and name.lower() != name: >>> raise QAPISemError( >>> info, "%s uses uppercase in name" % source) >>> + return match.group(3) >>> + >>> + >>> +def check_name_upper(name, info, source): >>> + stem = check_name_str(name, info, source, permit_upper=True) >>> + # TODO reject '[a-z-]' in @stem >>> + >> >> Creates (presumably) temporary errors in flake8 for the dead >> assignment here and below. > > All gone by the end of the series. > > "make check" and checkpatch were content. Anything else you'd like me > to run? > Eventually it'll be part of CI, with targets to run locally. I never expected the process to take this long, so I did not invest my time in developing an interim solution. I use a hastily written script to do my own testing, which I run for every commit that touches QAPI: #!/usr/bin/env bash set -e if [[ -f qapi/.flake8 ]]; then echo "flake8 --config=qapi/.flake8 qapi/" flake8 --config=qapi/.flake8 qapi/ fi if [[ -f qapi/pylintrc ]]; then echo "pylint --rcfile=qapi/pylintrc qapi/" pylint --rcfile=qapi/pylintrc qapi/ fi if [[ -f qapi/mypy.ini ]]; then echo "mypy --config-file=qapi/mypy.ini qapi/" mypy --config-file=qapi/mypy.ini qapi/ fi if [[ -f qapi/.isort.cfg ]]; then pushd qapi echo "isort -c ." isort -c . popd fi pushd ../bin/git make -j9 make check-qapi-schema popd
John Snow <jsnow@redhat.com> writes: > On 3/24/21 1:57 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 3/23/21 5:40 AM, Markus Armbruster wrote: >>>> Naming rules differ for the various kinds of names. To prepare >>>> enforcing them, define functions to check them: check_name_upper(), >>>> check_name_lower(), and check_name_camel(). For now, these merely >>>> wrap around check_name_str(), but that will change shortly. Replace >>>> the other uses of check_name_str() by appropriate uses of the >>>> wrappers. No change in behavior just yet. >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >>>> 1 file changed, 36 insertions(+), 15 deletions(-) >>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>>> index e00467636c..30285fe334 100644 >>>> --- a/scripts/qapi/expr.py >>>> +++ b/scripts/qapi/expr.py >>>> @@ -21,11 +21,12 @@ >>>> from .error import QAPISemError >>>> -# Names must be letters, numbers, -, and _. They must start >>>> with letter, >>>> -# except for downstream extensions which must start with __RFQDN_. >>>> -# Dots are only valid in the downstream extension prefix. >>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >>>> +# Names consist of letters, digits, -, and _, starting with a letter. >>>> +# An experimental name is prefixed with x-. A name of a downstream >>>> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >>>> + r'(x-)?' >>>> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >>>> def check_name_is_str(name, info, source): >>>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source, >>>> permit_upper=False): >>>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >>>> # and 'q_obj_*' implicit type names. >>>> - if not valid_name.match(name) or \ >>>> - c_name(name, False).startswith('q_'): >>>> + match = valid_name.match(name) >>>> + if not match or c_name(name, False).startswith('q_'): >>>> raise QAPISemError(info, "%s has an invalid name" % source) >>>> if not permit_upper and name.lower() != name: >>>> raise QAPISemError( >>>> info, "%s uses uppercase in name" % source) >>>> + return match.group(3) >>>> + >>>> + >>>> +def check_name_upper(name, info, source): >>>> + stem = check_name_str(name, info, source, permit_upper=True) >>>> + # TODO reject '[a-z-]' in @stem >>>> + >>> >>> Creates (presumably) temporary errors in flake8 for the dead >>> assignment here and below. >> >> All gone by the end of the series. >> >> "make check" and checkpatch were content. Anything else you'd like me >> to run? > > Eventually it'll be part of CI, with targets to run locally. > > I never expected the process to take this long, so I did not invest my > time in developing an interim solution. > > I use a hastily written script to do my own testing, which I run for > every commit that touches QAPI: > > #!/usr/bin/env bash > set -e > > if [[ -f qapi/.flake8 ]]; then > echo "flake8 --config=qapi/.flake8 qapi/" > flake8 --config=qapi/.flake8 qapi/ > fi > if [[ -f qapi/pylintrc ]]; then > echo "pylint --rcfile=qapi/pylintrc qapi/" > pylint --rcfile=qapi/pylintrc qapi/ > fi > if [[ -f qapi/mypy.ini ]]; then > echo "mypy --config-file=qapi/mypy.ini qapi/" > mypy --config-file=qapi/mypy.ini qapi/ > fi > > if [[ -f qapi/.isort.cfg ]]; then > pushd qapi > echo "isort -c ." > isort -c . > popd > fi > > pushd ../bin/git > make -j9 > make check-qapi-schema > popd Thanks for sharing this! Apropos qapi-gen testing scripts. I have scripts to show me how the generated code changes along the way in a branch. They evolved over a long time, and try to cope with changes in the tree that are hardly relevant anymore. By now, they could quite possibly make Frankenstein recoil in horror. As a secondary purpose, the scripts show me how output of pycodestyle-3 and pylint change. This would be uninteresting if the code in master was clean against a useful configuration of these tools. Your work has been making it less interesting.
On 3/25/21 2:18 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 3/24/21 1:57 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> On 3/23/21 5:40 AM, Markus Armbruster wrote: >>>>> Naming rules differ for the various kinds of names. To prepare >>>>> enforcing them, define functions to check them: check_name_upper(), >>>>> check_name_lower(), and check_name_camel(). For now, these merely >>>>> wrap around check_name_str(), but that will change shortly. Replace >>>>> the other uses of check_name_str() by appropriate uses of the >>>>> wrappers. No change in behavior just yet. >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>> --- >>>>> scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- >>>>> 1 file changed, 36 insertions(+), 15 deletions(-) >>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>>>> index e00467636c..30285fe334 100644 >>>>> --- a/scripts/qapi/expr.py >>>>> +++ b/scripts/qapi/expr.py >>>>> @@ -21,11 +21,12 @@ >>>>> from .error import QAPISemError >>>>> -# Names must be letters, numbers, -, and _. They must start >>>>> with letter, >>>>> -# except for downstream extensions which must start with __RFQDN_. >>>>> -# Dots are only valid in the downstream extension prefix. >>>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>>>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >>>>> +# Names consist of letters, digits, -, and _, starting with a letter. >>>>> +# An experimental name is prefixed with x-. A name of a downstream >>>>> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >>>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >>>>> + r'(x-)?' >>>>> + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >>>>> def check_name_is_str(name, info, source): >>>>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source, >>>>> permit_upper=False): >>>>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >>>>> # and 'q_obj_*' implicit type names. >>>>> - if not valid_name.match(name) or \ >>>>> - c_name(name, False).startswith('q_'): >>>>> + match = valid_name.match(name) >>>>> + if not match or c_name(name, False).startswith('q_'): >>>>> raise QAPISemError(info, "%s has an invalid name" % source) >>>>> if not permit_upper and name.lower() != name: >>>>> raise QAPISemError( >>>>> info, "%s uses uppercase in name" % source) >>>>> + return match.group(3) >>>>> + >>>>> + >>>>> +def check_name_upper(name, info, source): >>>>> + stem = check_name_str(name, info, source, permit_upper=True) >>>>> + # TODO reject '[a-z-]' in @stem >>>>> + >>>> >>>> Creates (presumably) temporary errors in flake8 for the dead >>>> assignment here and below. >>> >>> All gone by the end of the series. >>> >>> "make check" and checkpatch were content. Anything else you'd like me >>> to run? >> >> Eventually it'll be part of CI, with targets to run locally. >> >> I never expected the process to take this long, so I did not invest my >> time in developing an interim solution. >> >> I use a hastily written script to do my own testing, which I run for >> every commit that touches QAPI: >> >> #!/usr/bin/env bash >> set -e >> >> if [[ -f qapi/.flake8 ]]; then >> echo "flake8 --config=qapi/.flake8 qapi/" >> flake8 --config=qapi/.flake8 qapi/ >> fi >> if [[ -f qapi/pylintrc ]]; then >> echo "pylint --rcfile=qapi/pylintrc qapi/" >> pylint --rcfile=qapi/pylintrc qapi/ >> fi >> if [[ -f qapi/mypy.ini ]]; then >> echo "mypy --config-file=qapi/mypy.ini qapi/" >> mypy --config-file=qapi/mypy.ini qapi/ >> fi >> >> if [[ -f qapi/.isort.cfg ]]; then >> pushd qapi >> echo "isort -c ." >> isort -c . >> popd >> fi >> >> pushd ../bin/git >> make -j9 >> make check-qapi-schema >> popd > > Thanks for sharing this! > My intent, eventually, was to get scripts/qapi under python/qemu/qapi; under which there is a testing framework thing I have been debating on and off with Cleber for a while that does (basically) the same thing as what my hasty script does, but more integrated with Python ecosystem. It'll do a few things: (1) Gets these checks on CI (2) Allows developers to manually run the checks locally (3) Allows developers to manually run the checks locally using a pre-determined set of pinned linter versions to guarantee synchronicity with CI cd python/qemu && "make check" would do more or less the same as the hacky script above, "make venv-check" would create a virtual environment with precisely the right packages and then run the same. I have been stalled there for a while, and I missed freeze deadline from illness. Anguish. For now, the dumb script in my scripts directory does the lifting I need it to, using packages I have selected in my local environment that just-so-happen to pass. > Apropos qapi-gen testing scripts. I have scripts to show me how the > generated code changes along the way in a branch. They evolved over a > long time, and try to cope with changes in the tree that are hardly > relevant anymore. By now, they could quite possibly make Frankenstein > recoil in horror. > Are they in the tree? Largely if the generated code changes it's invisible to me, but I rely heavily on the unit tests. I guess maybe if they are not in a state to upstream it might not be worth the hassle to clean them, but I don't know. > As a secondary purpose, the scripts show me how output of pycodestyle-3 > and pylint change. This would be uninteresting if the code in master > was clean against a useful configuration of these tools. Your work has > been making it less interesting. > --js
John Snow <jsnow@redhat.com> writes: > On 3/25/21 2:18 AM, Markus Armbruster wrote: [...] >> Apropos qapi-gen testing scripts. I have scripts to show me how the >> generated code changes along the way in a branch. They evolved over a >> long time, and try to cope with changes in the tree that are hardly >> relevant anymore. By now, they could quite possibly make Frankenstein >> recoil in horror. >> > > Are they in the tree? No, because in their current state, they are incomprehensible *and* need frequent tinkering. > Largely if the generated code changes it's > invisible to me, but I rely heavily on the unit tests. I guess maybe if > they are not in a state to upstream it might not be worth the hassle to > clean them, but I don't know. The negative unit tests are fairly comprehensive, and guard against screwing up error path reasonably well. The positive unit tests compare the frontend state dumped by test-qapi.py, and compile-test the generated code. Reasonable protection against frontend screwups. Some protection against backend screwups. Plenty of unwanted code generation changes can go undetected. A tool to show generated code changes for review is useful, and having such a tool in the tree would be nice. >> As a secondary purpose, the scripts show me how output of pycodestyle-3 >> and pylint change. This would be uninteresting if the code in master >> was clean against a useful configuration of these tools. Your work has >> been making it less interesting. >> > > --js
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index e00467636c..30285fe334 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -21,11 +21,12 @@ from .error import QAPISemError -# Names must be letters, numbers, -, and _. They must start with letter, -# except for downstream extensions which must start with __RFQDN_. -# Dots are only valid in the downstream extension prefix. -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' - '[a-zA-Z][a-zA-Z0-9_-]*$') +# Names consist of letters, digits, -, and _, starting with a letter. +# An experimental name is prefixed with x-. A name of a downstream +# extension is prefixed with __RFQDN_. The latter prefix goes first. +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' + r'(x-)?' + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) def check_name_is_str(name, info, source): @@ -37,16 +38,38 @@ def check_name_str(name, info, source, permit_upper=False): # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. - if not valid_name.match(name) or \ - c_name(name, False).startswith('q_'): + match = valid_name.match(name) + if not match or c_name(name, False).startswith('q_'): raise QAPISemError(info, "%s has an invalid name" % source) if not permit_upper and name.lower() != name: raise QAPISemError( info, "%s uses uppercase in name" % source) + return match.group(3) + + +def check_name_upper(name, info, source): + stem = check_name_str(name, info, source, permit_upper=True) + # TODO reject '[a-z-]' in @stem + + +def check_name_lower(name, info, source, + permit_upper=False): + stem = check_name_str(name, info, source, permit_upper) + # TODO reject '_' in stem + + +def check_name_camel(name, info, source): + stem = check_name_str(name, info, source, permit_upper=True) + # TODO reject '[_-]' in stem, require CamelCase def check_defn_name_str(name, info, meta): - check_name_str(name, info, meta, permit_upper=True) + if meta == 'event': + check_name_upper(name, info, meta) + elif meta == 'command': + check_name_lower(name, info, meta, permit_upper=True) + else: + check_name_camel(name, info, meta) if name.endswith('Kind') or name.endswith('List'): raise QAPISemError( info, "%s name should not end in '%s'" % (meta, name[-4:])) @@ -163,8 +186,7 @@ def check_type(value, info, source, key_source = "%s member '%s'" % (source, key) if key.startswith('*'): key = key[1:] - check_name_str(key, info, key_source, - permit_upper=permit_upper) + check_name_lower(key, info, key_source, permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "%s uses reserved name" % key_source) check_keys(arg, info, key_source, ['type'], ['if', 'features']) @@ -186,7 +208,7 @@ def check_features(features, info): check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) - check_name_str(f['name'], info, source) + check_name_lower(f['name'], info, source) check_if(f, info, source) @@ -213,8 +235,7 @@ def check_enum(expr, info): # Enum members may start with a digit if member_name[0].isdigit(): member_name = 'd' + member_name # Hack: hide the digit - check_name_str(member_name, info, source, - permit_upper=permit_upper) + check_name_lower(member_name, info, source, permit_upper) check_if(member, info, source) @@ -244,7 +265,7 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key if discriminator is None: - check_name_str(key, info, source) + check_name_lower(key, info, source) # else: name is in discriminator enum, which gets checked check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) @@ -258,7 +279,7 @@ def check_alternate(expr, info): raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): source = "'data' member '%s'" % key - check_name_str(key, info, source) + check_name_lower(key, info, source) check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) check_type(value['type'], info, source)
Naming rules differ for the various kinds of names. To prepare enforcing them, define functions to check them: check_name_upper(), check_name_lower(), and check_name_camel(). For now, these merely wrap around check_name_str(), but that will change shortly. Replace the other uses of check_name_str() by appropriate uses of the wrappers. No change in behavior just yet. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 15 deletions(-)