John Snow <js...@redhat.com> writes:
> On 3/24/21 2:22 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <arm...@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?
>>
>
> The long names are nice and descriptive.
Then I should give them a try to see whether the result feels neat or
ugly.
