Eric Blake <ebl...@redhat.com> writes:
> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> Starting error messages with a capital letter complicates things when
>> text can get interpolated both at the beginning and in the middle of
>> an error message. The next patch will do that. Switch to lower case
>> to keep it simpler.
>>
>> For what it's worth, the GNU Coding Standards advise the message
>> "should not begin with a capital letter when it follows a program name
>> and/or file name, because that isn’t the beginning of a sentence. (The
>> sentence conceptually starts at the beginning of the line.)"
>
> We're inconsistent throughout the code base, but this is one place where
> I like the GCS rationale. Fixing it everywhere may not be worth the
> churn, but fixing it within the subset of the qapi generator is worthwhile.
>
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> scripts/qapi/common.py | 175 +++++++++---------
>> tests/qapi-schema/alternate-any.err | 2 +-
>
>> tests/qapi-schema/unknown-expr-key.err | 2 +-
>> 125 files changed, 215 insertions(+), 204 deletions(-)
>> create mode 100644 tests/qapi-schema/escape-too-big.err
>> create mode 100644 tests/qapi-schema/string-control.err
>> create mode 100644 tests/qapi-schema/string-unclosed.err
>> create mode 100644 tests/qapi-schema/string-unicode.err
>
> Umm, what's going on here?
Accident.
> You'll want to either drop these files (if they were leftovers in your
> working directory from previous points in time), or defer their addition
> to when the corresponding actual tests exist.
I'll drop them,
>> def get_doc(self, info):
>> if self.val != '##':
>> - raise QAPIParseError(self, "Junk after '##' at start of "
>> - "documentation comment")
>> + raise QAPIParseError(
>> + self, "junk after '##' at start of documentation comment")
>
> Reformatting like this also makes grepping for a particular message easier.
>
>
>> @@ -868,8 +869,8 @@ def check_union(expr, info):
>> enum_values = members.keys()
>> allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum']
>> if base is not None:
>> - raise QAPISemError(info, "Simple union '%s' must not have a
>> base" %
>> - name)
>> + raise QAPISemError(
>> + info, "simple union '%s' must not have a base" % name)
>>
>
> A bit odd that you reformat here to get the second argument all on one
> line...
>
>> # Else, it's a flat union.
>> else:
>> @@ -878,46 +879,47 @@ def check_union(expr, info):
>> base, allow_dict=name,
>> allow_metas=['struct'])
>> if not base:
>> - raise QAPISemError(info, "Flat union '%s' must have a base"
>> + raise QAPISemError(info, "flat union '%s' must have a base"
>> % name)
>
> ...but not here. The reformatting is not the primary focus of the
> patch, and doesn't hurt semantically whether or not you do it, but maybe
> it is worth calling out in the commit message the criteria you used for
> deciding when to reformat, and/or make the patch strive for more
> consistency.
I admit I wobble between
raise QAPISemError(info, "some lengthy error message text with %s"
% argument)
and
raise QAPISemError(info,
"some lengthy error message text with %s" % argument)
and
raise QAPISemError(
info, "some lengthy error message text with %s" % argument)
The first looks okay, but as a rule, lines should be broken at an
operator with lowest precedence.
The second tends to produce long lines.
I'm still getting used to the third.
> I'll leave that up to you; fixing the spurious new files,
> and making your choice of where to place the linebreaks, doesn't affect
> my ability to offer:
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
I'll have another look.
Thanks!
