Re: [PATCH 13/28] qapi: Enforce event naming rules

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> Event names should be ALL_CAPS with words separated by underscore.
>> Enforce this.  The only offenders are in tests/.  Fix them.  Existing
>> test event-case covers the new error.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/unit/test-qmp-event.c               |  6 +++---
>>   scripts/qapi/expr.py                      |  4 +++-
>>   tests/qapi-schema/doc-good.json           |  4 ++--
>>   tests/qapi-schema/doc-good.out            |  4 ++--
>>   tests/qapi-schema/doc-good.txt            |  2 +-
>>   tests/qapi-schema/doc-invalid-return.json |  4 ++--
>>   tests/qapi-schema/event-case.err          |  2 ++
>>   tests/qapi-schema/event-case.json         |  2 --
>>   tests/qapi-schema/event-case.out          | 14 --------------
>>   tests/qapi-schema/qapi-schema-test.json   |  6 +++---
>>   tests/qapi-schema/qapi-schema-test.out    |  8 ++++----
>>   11 files changed, 22 insertions(+), 34 deletions(-)
>> diff --git a/tests/unit/test-qmp-event.c
>> b/tests/unit/test-qmp-event.c
>> index 047f44ff9a..d58c3b78f2 100644
>> --- a/tests/unit/test-qmp-event.c
>> +++ b/tests/unit/test-qmp-event.c
>> @@ -143,7 +143,7 @@ static void test_event_d(TestEventData *data,
>>     static void test_event_deprecated(TestEventData *data, const
>> void *unused)
>>   {
>> -    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }");
>> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1' }");
>>         memset(&compat_policy, 0, sizeof(compat_policy));
>>   @@ -163,7 +163,7 @@ static void
>> test_event_deprecated_data(TestEventData *data, const void *unused)
>>   {
>>       memset(&compat_policy, 0, sizeof(compat_policy));
>>   -    data->expect = qdict_from_jsonf_nofail("{ 'event':
>> 'TEST-EVENT-FEATURES0',"
>> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
>>                                              " 'data': { 'foo': 42 } }");
>>       qapi_event_send_test_event_features0(42);
>>       g_assert(data->emitted);
>> @@ -172,7 +172,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused)
>>         compat_policy.has_deprecated_output = true;
>>       compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
>> -    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0' }");
>> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }");
>>       qapi_event_send_test_event_features0(42);
>>       g_assert(data->emitted);
>>   diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index b5fb0be48b..c065505b27 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -45,7 +45,9 @@ def check_name_str(name, info, source):
>>     def check_name_upper(name, info, source):
>>       stem = check_name_str(name, info, source)
>> -    # TODO reject '[a-z-]' in @stem
>> +    if re.search(r'[a-z-]', stem):
>> +        raise QAPISemError(
>> +            info, "name of %s must not use lowercase or '-'" % source)
>>   
>
> Does a little bit more than check_name_upper. Is this only used for
> event names? I guess so. Should it be inlined into check_defn_name_str 
> instead in this case, or nah?

I'd prefer not to inline.  I'm open to better function names.

We have three name styles.  qapi-code-gen.txt:

    [Type] definitions should always use CamelCase for
    user-defined type names, while built-in types are lowercase.

    [...]

    Command names, and member names within a type, should be all lower
    case with words separated by a hyphen.  [...]

    Event names should be ALL_CAPS with words separated by underscore.

I define three functions for them: check_name_camel(),
check_name_lower(), and check_name_upper().

The functions factor out the naming rule aspect, and they let us keep
the naming rule aspect together.  That's why I'd prefer not to inline.

We could name them after their purpose instead:
check_name_user_defined_type(), check_name_command_or_member(),
check_name_event().  The first two are rather long.  Shorter:
check_name_type(), check_name_other(), check_name_event().

Thoughts?