| Message ID | 20230420102619.348173-3-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: allow unions to contain further unions | expand |
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?
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 With regards, Daniel
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?
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
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)
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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- scripts/qapi/schema.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)