Re: [PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Daniel P . Berrangé]

On Tue, Apr 25, 2023 at 03:17:52PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 24, 2023 at 01:38:21PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > When describing member types always include the context of the
> >> > containing type. Although this is often redundant, in some cases
> >> > it will help to reduce ambiguity.
> >> 
> >> This is no longer true.  It was in v2.  Suggest:
> >> 
> >>   Error messages describe object members, enumeration values, features,
> >>   and variants like ROLE 'NAME', where ROLE is "member", "value",
> >>   "feature", or "branch", respectively.  When the member is defined in
> >>   another type, e.g. inherited from a base type, we add "of type
> >>   'TYPE'".  Example: test case struct-base-clash-deep reports a member
> >>   of type 'Sub' clashing with a member of its base type 'Base' as
> >> 
> >>       struct-base-clash-deep.json: In struct 'Sub':
> >>       struct-base-clash-deep.json:10: member 'name' collides with member 
> >> 'name' of type 'Base'
> >> 
> >>   Members of implicitly defined types need special treatment.  We don't
> >>   want to add "of type 'TYPE'" for them, because their named are made up
> >>   and mean nothing to the user.  Instead, we describe members of an
> >>   implicitly defined base type as "base member 'NAME'", and command and
> >>   event parameters as "parameter 'NAME'".  Example: test case
> >>   union-bad-base reports member of a variant's type clashing with a
> >>   member of its implicitly defined base type as
> >> 
> >>       union-bad-base.json: In union 'TestUnion':
> >>       union-bad-base.json:8: member 'string' of type 'TestTypeA' collides 
> >> with base member 'string'
> >> 
> >>   The next commit will permit unions as variant types.  "base member
> >>   'NAME' would then be ambigious: is it the union's base, or is it the
> >>   union's variant's base?  One of its test cases would report a clash
> >>   between two such bases as "base member 'type' collides with base
> >>   member 'type'".  Confusing.
> >> 
> >>   Refine the special treatment: add "of TYPE" even for implicitly
> >>   defined types, but massage TYPE and ROLE so they make sense for the
> >>   user.
> >> 
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >  scripts/qapi/schema.py | 9 +++++++--
> >> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index 207e4d71f3..da04b97ded 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -697,6 +697,7 @@ def connect_doc(self, doc):
> >> >  
> >> >      def describe(self, info):
> >> >          role = self.role
> >> > +        meta = 'type'
> >> >          defined_in = self.defined_in
> >> >          assert defined_in
> >> >  
> >> > @@ -708,13 +709,17 @@ def describe(self, info):
> >> >                  # Implicit type created for a command's dict 'data'
> >> >                  assert role == 'member'
> >> >                  role = 'parameter'
> >> > +                meta = 'command'
> >> > +                defined_in = defined_in[:-4]
> >> >              elif defined_in.endswith('-base'):
> >> >                  # Implicit type created for a union's dict 'base'
> >> >                  role = 'base ' + role
> >> > +                defined_in = defined_in[:-5]
> >> >              else:
> >> >                  assert False
> >> > -        elif defined_in != info.defn_name:
> >> > -            return "%s '%s' of type '%s'" % (role, self.name, 
> >> > defined_in)
> >> > +
> >> > +        if defined_in != info.defn_name:
> >> > +            return "%s '%s' of %s '%s'" % (role, self.name, meta, 
> >> > defined_in)
> >> >          return "%s '%s'" % (role, self.name)
> >> 
> >> Since I rewrote both the patch and the commit message, would you like me
> >> to take the blame and claim authorship?
> >
> > Yes, I should have credited you as the author here since it was just
> > taking your proposed code. The suggested commit message looks fine too
> 
> Thanks!  May I add your R-by in my tree?

Certainly

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|