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

[John Snow]

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

I tried my hand at documenting them in my respin; I am not entirely 
confident I got the names and purposes and semantics exactly right. I 
didn't try to rename them, but it would be easy to do. You'll have to 
let me know your preferences.

--js