| Message ID | 20171002152552.27999-20-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Command line QAPIfication | expand |
On Mon, Oct 2, 2017 at 5:25 PM, Markus Armbruster <armbru@redhat.com> wrote: > The QAPI schema parser has always accepted only single-quoted strings, > even though JSON strings are double-quoted. Accept double-quoted > strings as well, so you can write strings containing single quotes > without backslash escapes. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> What's the motivation to allow both? If we were to switch from single to double quote only, that would make more sense. otherwise, patch looks good > --- > docs/devel/qapi-code-gen.txt | 2 +- > scripts/qapi.py | 8 +++++--- > tests/qapi-schema/qapi-schema-test.json | 2 +- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index 3186c36460..835c641ea8 100644 > --- a/docs/devel/qapi-code-gen.txt > +++ b/docs/devel/qapi-code-gen.txt > @@ -32,7 +32,7 @@ differences: > > * No JSON numbers > > -* Strings use 'single quotes' instead of "double quotes" > +* Strings can use 'single quotes' in addition to "double quotes" > > * The input character set is plain ASCII > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 477402b7f8..18c8175866 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -382,14 +382,15 @@ class QAPISchemaParser(object): > return > elif self.tok in '{}:,[]': > return > - elif self.tok == "'": > + elif self.tok == "'" or self.tok == '"': > string = '' > esc = False > while True: > ch = self.src[self.cursor] > self.cursor += 1 > if ch == '\n': > - raise QAPIParseError(self, 'Missing terminating "\'"') > + raise QAPIParseError( > + self, 'Missing terminating %r' % self.tok) > if esc: > if ch == 'b': > string += '\b' > @@ -429,8 +430,9 @@ class QAPISchemaParser(object): > esc = False > elif ch == '\\': > esc = True > - elif ch == "'": > + elif ch == self.tok: > self.val = string > + self.tok = "'" > return > else: > string += ch > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index ac8aefc924..c74d5632a5 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -11,7 +11,7 @@ > 'guest-sync' ] } } > > { 'struct': 'TestStruct', > - 'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } } > + 'data': { 'integer': 'int', 'boolean': 'bool', 'string': "str" } } > > # for testing enums > { 'struct': 'NestedEnumsOne', > -- > 2.13.6 > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > On Mon, Oct 2, 2017 at 5:25 PM, Markus Armbruster <armbru@redhat.com> wrote: >> The QAPI schema parser has always accepted only single-quoted strings, >> even though JSON strings are double-quoted. Accept double-quoted >> strings as well, so you can write strings containing single quotes >> without backslash escapes. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > What's the motivation to allow both? If we were to switch from single > to double quote only, that would make more sense. Abandoning single quotes now would require us to touch pretty much every line of code in the schemas. I don't think correcting quotes is worth wrecking git-blame. Sadly, the schema language is neither JSON, nor an established extension of JSON, nor Python. This commit brings the schema language one step closer to a superset of JSON. I feel "homegrown superset" is a slightly less bad idea than "homegrown with large overlap". Naming the schema files .json was in bad taste. > otherwise, patch looks good Ready to upgrade to R-by now? Want me to work more of my rationale into the commit message?
On Thu, Oct 5, 2017 at 6:41 AM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > >> On Mon, Oct 2, 2017 at 5:25 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> The QAPI schema parser has always accepted only single-quoted strings, >>> even though JSON strings are double-quoted. Accept double-quoted >>> strings as well, so you can write strings containing single quotes >>> without backslash escapes. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> What's the motivation to allow both? If we were to switch from single >> to double quote only, that would make more sense. > > Abandoning single quotes now would require us to touch pretty much every > line of code in the schemas. I don't think correcting quotes is worth > wrecking git-blame. > Recent (and upcoming) changes to the schema are already quite invasive. I think we could do it, convert all strings to double-quote, and it would help with getting the schema closer to a valid json. Fwiw, there are tools like https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html to skip commits in git-blame. It's also fairly easy to run git blame before the reformatting commit. > Sadly, the schema language is neither JSON, nor an established extension > of JSON, nor Python. This commit brings the schema language one step > closer to a superset of JSON. I feel "homegrown superset" is a slightly > less bad idea than "homegrown with large overlap". > > Naming the schema files .json was in bad taste. > >> otherwise, patch looks good > > Ready to upgrade to R-by now? > > Want me to work more of my rationale into the commit message?
On 10/04/2017 11:41 PM, Markus Armbruster wrote: > Sadly, the schema language is neither JSON, nor an established extension > of JSON, nor Python. This commit brings the schema language one step > closer to a superset of JSON. I feel "homegrown superset" is a slightly > less bad idea than "homegrown with large overlap". > > Naming the schema files .json was in bad taste. Would it make sense to rename all of our files from .json to .qapi? Then it is obvious that we are using a homegrown syntax; and it is also easy enough to tweak things like .dir-locals.el to recognize that suffix as triggering specific formatting rules. Git rename detection means it is still reasonable to blame across file renames.
Eric Blake <eblake@redhat.com> writes: > On 10/04/2017 11:41 PM, Markus Armbruster wrote: > >> Sadly, the schema language is neither JSON, nor an established extension >> of JSON, nor Python. This commit brings the schema language one step >> closer to a superset of JSON. I feel "homegrown superset" is a slightly >> less bad idea than "homegrown with large overlap". >> >> Naming the schema files .json was in bad taste. > > Would it make sense to rename all of our files from .json to .qapi? > Then it is obvious that we are using a homegrown syntax; and it is also > easy enough to tweak things like .dir-locals.el to recognize that suffix > as triggering specific formatting rules. Git rename detection means it > is still reasonable to blame across file renames. I don't know. I'm always reluctant to rename files. Probably too reluctant. Opinions?
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > On Thu, Oct 5, 2017 at 6:41 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >>> On Mon, Oct 2, 2017 at 5:25 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> The QAPI schema parser has always accepted only single-quoted strings, >>>> even though JSON strings are double-quoted. Accept double-quoted >>>> strings as well, so you can write strings containing single quotes >>>> without backslash escapes. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> >>> What's the motivation to allow both? If we were to switch from single >>> to double quote only, that would make more sense. >> >> Abandoning single quotes now would require us to touch pretty much every >> line of code in the schemas. I don't think correcting quotes is worth >> wrecking git-blame. >> > > Recent (and upcoming) changes to the schema are already quite > invasive. I think we could do it, convert all strings to double-quote, > and it would help with getting the schema closer to a valid json. Is the recent (and upcoming) churn *that* bad? Got numbers? > Fwiw, there are tools like > https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html > to skip commits in git-blame. It's also fairly easy to run git blame > before the reformatting commit. Both techniques add friction... >> Sadly, the schema language is neither JSON, nor an established extension >> of JSON, nor Python. This commit brings the schema language one step >> closer to a superset of JSON. I feel "homegrown superset" is a slightly >> less bad idea than "homegrown with large overlap". >> >> Naming the schema files .json was in bad taste. >> >>> otherwise, patch looks good >> >> Ready to upgrade to R-by now? >> >> Want me to work more of my rationale into the commit message?
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 3186c36460..835c641ea8 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -32,7 +32,7 @@ differences: * No JSON numbers -* Strings use 'single quotes' instead of "double quotes" +* Strings can use 'single quotes' in addition to "double quotes" * The input character set is plain ASCII diff --git a/scripts/qapi.py b/scripts/qapi.py index 477402b7f8..18c8175866 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -382,14 +382,15 @@ class QAPISchemaParser(object): return elif self.tok in '{}:,[]': return - elif self.tok == "'": + elif self.tok == "'" or self.tok == '"': string = '' esc = False while True: ch = self.src[self.cursor] self.cursor += 1 if ch == '\n': - raise QAPIParseError(self, 'Missing terminating "\'"') + raise QAPIParseError( + self, 'Missing terminating %r' % self.tok) if esc: if ch == 'b': string += '\b' @@ -429,8 +430,9 @@ class QAPISchemaParser(object): esc = False elif ch == '\\': esc = True - elif ch == "'": + elif ch == self.tok: self.val = string + self.tok = "'" return else: string += ch diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index ac8aefc924..c74d5632a5 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -11,7 +11,7 @@ 'guest-sync' ] } } { 'struct': 'TestStruct', - 'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } } + 'data': { 'integer': 'int', 'boolean': 'bool', 'string': "str" } } # for testing enums { 'struct': 'NestedEnumsOne',
The QAPI schema parser has always accepted only single-quoted strings, even though JSON strings are double-quoted. Accept double-quoted strings as well, so you can write strings containing single quotes without backslash escapes. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/devel/qapi-code-gen.txt | 2 +- scripts/qapi.py | 8 +++++--- tests/qapi-schema/qapi-schema-test.json | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)