Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking

[John Snow <jsnow@redhat.com>]

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