On Mon, Apr 24, 2023 at 01:38:21PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@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é <berra...@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
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 :|
