| Message ID | 20210325060356.4040114-3-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
John Snow <jsnow@redhat.com> writes: > PEP8's BDFL writes: "For flowing long blocks of text with fewer > structural restrictions (docstrings or comments), the line length should > be limited to 72 characters." > > I do not like this patch. I have included it explicitly to recommend we > do not pay any further heed to the 72 column limit. > > Signed-off-by: John Snow <jsnow@redhat.com> I'd like to get the remainder of this series moving again before digging into this patch.
On 3/25/21 11:21 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> PEP8's BDFL writes: "For flowing long blocks of text with fewer >> structural restrictions (docstrings or comments), the line length should >> be limited to 72 characters." >> >> I do not like this patch. I have included it explicitly to recommend we >> do not pay any further heed to the 72 column limit. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > > I'd like to get the remainder of this series moving again before digging > into this patch. > I am dropping it, then -- I have no interest in bringing a patch I dislike along for another respin. --js
John Snow <jsnow@redhat.com> writes: > On 3/25/21 11:21 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>> structural restrictions (docstrings or comments), the line length should >>> be limited to 72 characters." >>> >>> I do not like this patch. I have included it explicitly to recommend we >>> do not pay any further heed to the 72 column limit. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >> >> I'd like to get the remainder of this series moving again before digging >> into this patch. > > I am dropping it, then -- I have no interest in bringing a patch I > dislike along for another respin. Despite your dislike, there might be good parts, and if there are, I'd like to mine them. I don't need you to track the patch for that, though. Feel free to drop it. Thank you for exploring the max-doc-length option.
On 3/26/21 2:26 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 3/25/21 11:21 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>>> structural restrictions (docstrings or comments), the line length should >>>> be limited to 72 characters." >>>> >>>> I do not like this patch. I have included it explicitly to recommend we >>>> do not pay any further heed to the 72 column limit. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>> >>> I'd like to get the remainder of this series moving again before digging >>> into this patch. >> >> I am dropping it, then -- I have no interest in bringing a patch I >> dislike along for another respin. > > Despite your dislike, there might be good parts, and if there are, I'd > like to mine them. I don't need you to track the patch for that, > though. Feel free to drop it. > > Thank you for exploring the max-doc-length option. > Being less terse about it: Mostly, I don't like how it enforces this column width even for indented structures. Generally, we claim that 72 columns is "comfortable to read" and I agree. However, when we start in a margin, I am not convinced that this is actually more readable than the alternative. We aren't using our full 72 characters here. For personal projects I tend to relax the column limit to about 100 chars, which gives nice breathing room and generally reduces the edge cases for error strings and so on. (Not suggesting we do that here so long as we remain on a mailing-list based workflow.) I can't say I am a fan of the limit; I don't think it's something I can reasonably enforce for python/* so I have some concerns over consistency, so I think it'd be easier to just not. I *did* try, though; I just think it brought up too many judgment calls for how to make single-line comments not look super awkward. I imagine it'll cause similar delays for other authors, and exasperated sighs when the CI fails due to a 73-column comment.
On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: > Being less terse about it: Mostly, I don't like how it enforces this > column width even for indented structures. Generally, we claim that 72 > columns is "comfortable to read" and I agree. > > However, when we start in a margin, I > am not convinced that this is > actually more readable than the > alternative. We aren't using our full > 72 characters here. I agree, and I don't see any strong reason to hold our Python code to a different standard to the rest of our codebase as regards line length and comment standards. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: >> Being less terse about it: Mostly, I don't like how it enforces this >> column width even for indented structures. Generally, we claim that 72 >> columns is "comfortable to read" and I agree. >> >> However, when we start in a margin, I >> am not convinced that this is >> actually more readable than the >> alternative. We aren't using our full >> 72 characters here. > > I agree, and I don't see any strong reason to hold our Python > code to a different standard to the rest of our codebase as > regards line length and comment standards. I can't see much of a conflict between canonical Python style and the rest of our code. (If there was a conflict, then I'd doubt we should hold our Python code to a different standard than pretty much all the other Python code out there.) PEP 8 is expressly a *guide*. It doesn't want to be treated as law. It tells you when to ignore its guidance even before it gives any, right in the second section. Applicable part: Some other good reasons to ignore a particular guideline: 1. When applying the guideline would make the code less readable, even for someone who is used to reading code that follows this PEP. Going beyond 72 colums to make the comment more readable is exactly what PEP 8 wants you to do. This is no excuse for going beyond when you could just as well break the line earlier. There's a reason pycodestyle distinguishes between errors and warnings, and "line too long" is a warning. We've been conditioned to conflate warnings with errors by C's "the standard permits it, but you really shouldn't" warnings. However, treating style warnings as errors is exactly what PEP 8 calls a folly of little minds.
John Snow <jsnow@redhat.com> writes: > On 3/26/21 2:26 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 3/25/21 11:21 AM, Markus Armbruster wrote: >>>> John Snow <jsnow@redhat.com> writes: >>>> >>>>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>>>> structural restrictions (docstrings or comments), the line length should >>>>> be limited to 72 characters." >>>>> >>>>> I do not like this patch. I have included it explicitly to recommend we >>>>> do not pay any further heed to the 72 column limit. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> >>>> I'd like to get the remainder of this series moving again before digging >>>> into this patch. >>> >>> I am dropping it, then -- I have no interest in bringing a patch I >>> dislike along for another respin. >> Despite your dislike, there might be good parts, and if there are, >> I'd >> like to mine them. I don't need you to track the patch for that, >> though. Feel free to drop it. >> Thank you for exploring the max-doc-length option. >> > > Being less terse about it: Mostly, I don't like how it enforces this > column width even for indented structures. Generally, we claim that 72 > columns is "comfortable to read" and I agree. > > However, when we start in a margin, I > am not convinced that this is > actually more readable than the > alternative. We aren't using our full > 72 characters here. > > For personal projects I tend to relax the column limit to about 100 > chars, which gives nice breathing room and generally reduces the edge > cases for error strings and so on. (Not suggesting we do that here so > long as we remain on a mailing-list based workflow.) > > I can't say I am a fan of the limit; I don't think it's something I > can reasonably enforce for python/* so I have some concerns over > consistency, so I think it'd be easier to just not. I'm with PEP 8 here: go beyond the line length limits juidicously, not carelessly. This cannot be enforced automatically with the tools we have. > I *did* try, though; I just think it brought up too many judgment > calls for how to make single-line comments not look super awkward. I > imagine it'll cause similar delays for other authors, and exasperated > sighs when the CI fails due to a 73-column comment. Enforcing a hard 72 limit in CI would be precisely what PEP 8 does not want us to do.
On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: > On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: > > Being less terse about it: Mostly, I don't like how it enforces this > > column width even for indented structures. Generally, we claim that 72 > > columns is "comfortable to read" and I agree. > > > > However, when we start in a margin, I > > am not convinced that this is > > actually more readable than the > > alternative. We aren't using our full > > 72 characters here. > > I agree, and I don't see any strong reason to hold our Python > code to a different standard to the rest of our codebase as > regards line length and comment standards. There's one small difference with python vs the rest of the codebase when it comes to API doc strings specifically. eg we have a docstring API comment in python/qemu/machine.py: class QEMUMachine: """ A QEMU VM. Use this object as a context manager to ensure the QEMU process terminates:: with VM(binary) as vm: ... # vm is guaranteed to be shut down here """ This formatting, including line breaks, is preserved as-is when a user requests viewing of the help: >>> print(help(qemu.machine.QEMUMachine)) Help on class QEMUMachine in module qemu.machine: class QEMUMachine(builtins.object) | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) | | A QEMU VM. | | Use this object as a context manager to ensure | the QEMU process terminates:: | | with VM(binary) as vm: | ... | # vm is guaranteed to be shut down here | | Methods defined here: | IOW, while we as QEMU maintainers may not care about keeping to a narrow line width, with API docstrings, we're also declaring that none of the users of the python APIs can care either. These docstrings are never reflowed, so they can end up wrapping if the user's terminal is narrow which looks very ugly. So this python API docstring scenario is slightly different from our main codebase, where majority of comments are only ever going to be seen by QEMU maintainers, and where C API doc strings don't preserve formatting, because they're turned into HTML and re-flowed. Having said all that, I still don't think we need to restrict ourselves to 72 characters. This is not the 1980's with people using text terminals with physical size constraints. I think it is fine if we let python docstrings get larger - especially if the docstrings are already indented 4/8/12 spaces due to the code indent context, because the code indent is removed when comments are displayed. I think a 100 char line limit would be fine and still not cause wrapping when using python live help(). Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: >> On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: >> > Being less terse about it: Mostly, I don't like how it enforces this >> > column width even for indented structures. Generally, we claim that 72 >> > columns is "comfortable to read" and I agree. >> > >> > However, when we start in a margin, I >> > am not convinced that this is >> > actually more readable than the >> > alternative. We aren't using our full >> > 72 characters here. >> >> I agree, and I don't see any strong reason to hold our Python >> code to a different standard to the rest of our codebase as >> regards line length and comment standards. > > There's one small difference with python vs the rest of the codebase when > it comes to API doc strings specifically. eg we have a docstring API comment > in python/qemu/machine.py: > > class QEMUMachine: > """ > A QEMU VM. > > Use this object as a context manager to ensure > the QEMU process terminates:: > > with VM(binary) as vm: > ... > # vm is guaranteed to be shut down here > """ > > This formatting, including line breaks, is preserved as-is when a user > requests viewing of the help: > >>>> print(help(qemu.machine.QEMUMachine)) > > Help on class QEMUMachine in module qemu.machine: > > class QEMUMachine(builtins.object) > | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) > | > | A QEMU VM. > | > | Use this object as a context manager to ensure > | the QEMU process terminates:: > | > | with VM(binary) as vm: > | ... > | # vm is guaranteed to be shut down here > | > | Methods defined here: > | > > > IOW, while we as QEMU maintainers may not care about keeping to a narrow > line width, with API docstrings, we're also declaring that none of the > users of the python APIs can care either. These docstrings are never > reflowed, so they can end up wrapping if the user's terminal is narrow > which looks very ugly. > > > So this python API docstring scenario is slightly different from our > main codebase, where majority of comments are only ever going to be seen > by QEMU maintainers, and where C API doc strings don't preserve formatting, > because they're turned into HTML and re-flowed. > > Having said all that, I still don't think we need to restrict ourselves > to 72 characters. This is not the 1980's with people using text terminals > with physical size constraints. I think it is fine if we let python > docstrings get larger - especially if the docstrings are already indented > 4/8/12 spaces due to the code indent context, because the code indent is > removed when comments are displayed. I think a 100 char line limit would > be fine and still not cause wrapping when using python live help(). The trouble with long lines is not text terminals, it's humans. Humans tend to have trouble following long lines with their eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[*]. Most doc strings are indented once (classes, functions) or twice (methods). 72 - 8 is roughly 60. With nesting, doc strings can become indented more. Nesting sufficient to squeeze the doc string width to column 72 under roughly 60 is pretty rare. Going beyond 72 colums to keep such doc strings readable is exactly what PEP 8 wants you to do. Again, I see no reason to deviate from PEP 8. [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
On 4/9/21 5:33 AM, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: >>> On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: >>>> Being less terse about it: Mostly, I don't like how it enforces this >>>> column width even for indented structures. Generally, we claim that 72 >>>> columns is "comfortable to read" and I agree. >>>> >>>> However, when we start in a margin, I >>>> am not convinced that this is >>>> actually more readable than the >>>> alternative. We aren't using our full >>>> 72 characters here. >>> >>> I agree, and I don't see any strong reason to hold our Python >>> code to a different standard to the rest of our codebase as >>> regards line length and comment standards. >> >> There's one small difference with python vs the rest of the codebase when >> it comes to API doc strings specifically. eg we have a docstring API comment >> in python/qemu/machine.py: >> >> class QEMUMachine: >> """ >> A QEMU VM. >> >> Use this object as a context manager to ensure >> the QEMU process terminates:: >> >> with VM(binary) as vm: >> ... >> # vm is guaranteed to be shut down here >> """ >> >> This formatting, including line breaks, is preserved as-is when a user >> requests viewing of the help: >> >>>>> print(help(qemu.machine.QEMUMachine)) >> >> Help on class QEMUMachine in module qemu.machine: >> >> class QEMUMachine(builtins.object) >> | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) >> | >> | A QEMU VM. >> | >> | Use this object as a context manager to ensure >> | the QEMU process terminates:: >> | >> | with VM(binary) as vm: >> | ... >> | # vm is guaranteed to be shut down here >> | >> | Methods defined here: >> | >> >> >> IOW, while we as QEMU maintainers may not care about keeping to a narrow >> line width, with API docstrings, we're also declaring that none of the >> users of the python APIs can care either. These docstrings are never >> reflowed, so they can end up wrapping if the user's terminal is narrow >> which looks very ugly. >> >> >> So this python API docstring scenario is slightly different from our >> main codebase, where majority of comments are only ever going to be seen >> by QEMU maintainers, and where C API doc strings don't preserve formatting, >> because they're turned into HTML and re-flowed. >> >> Having said all that, I still don't think we need to restrict ourselves >> to 72 characters. This is not the 1980's with people using text terminals >> with physical size constraints. I think it is fine if we let python >> docstrings get larger - especially if the docstrings are already indented >> 4/8/12 spaces due to the code indent context, because the code indent is >> removed when comments are displayed. I think a 100 char line limit would >> be fine and still not cause wrapping when using python live help(). > > The trouble with long lines is not text terminals, it's humans. Humans > tend to have trouble following long lines with their eyes (I sure do). > Typographic manuals suggest to limit columns to roughly 60 characters > for exactly that reason[*]. > > Most doc strings are indented once (classes, functions) or twice > (methods). 72 - 8 is roughly 60. > My problem with this patch isn't actually the docstrings -- it's one-line comments. If you can teach flake8 to allow this: # Pretend this is a single-line comment that's 73 chars but disallow this: # Pretend this is a two-line comment that's 73 chars, # and continues to a new line that's also pretty long, # and maybe keeps going, too. I will happily accept that patch. Without the ability to enforce the style though, I am reluctant to pretend that it's even a preference that we have. I think it's a waste to hunt down and re-flow single-line comments that just barely squeak over a limit. They look worse. We can discuss this more when we go to propose a style guide for the Python folder; I think it's maybe a misprioritization of our energies in the present context. (I still have the style guide on my TODO list, and even began writing a draft at one point, but I think we'd both like to press forward on the Typing bits first.) > With nesting, doc strings can become indented more. Nesting sufficient > to squeeze the doc string width to column 72 under roughly 60 is pretty > rare. Going beyond 72 colums to keep such doc strings readable is > exactly what PEP 8 wants you to do. > > Again, I see no reason to deviate from PEP 8. > > > [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style >
John Snow <jsnow@redhat.com> writes: > PEP8's BDFL writes: "For flowing long blocks of text with fewer > structural restrictions (docstrings or comments), the line length should > be limited to 72 characters." > > I do not like this patch. I have included it explicitly to recommend we > do not pay any further heed to the 72 column limit. Let me go through the patch hunk by hunk to see what I like and what I don't like. In case you'd prefer not to pay any further heed to line length: please check out my comment on c_name() anyway. It's about doc string style, and relevant regardless of line length limits. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/.flake8 | 1 + > scripts/qapi/common.py | 8 +++++--- > scripts/qapi/events.py | 9 +++++---- > scripts/qapi/gen.py | 8 ++++---- > scripts/qapi/introspect.py | 8 +++++--- > scripts/qapi/main.py | 4 ++-- > scripts/qapi/parser.py | 15 ++++++++------- > scripts/qapi/schema.py | 23 +++++++++++++---------- > scripts/qapi/types.py | 7 ++++--- > 9 files changed, 47 insertions(+), 36 deletions(-) > > diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 > index 6b158c68b8..4f00455290 100644 > --- a/scripts/qapi/.flake8 > +++ b/scripts/qapi/.flake8 > @@ -1,2 +1,3 @@ > [flake8] > extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's > +max-doc-length = 72 > \ No newline at end of file Since we intend to make use of PEP 8's license to go over the line length limit, having the build gripe about it is not useful. Drop. > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index cbd3fd81d3..6e3d9b8ecd 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: > length = len(c_fun_str) > for i in range(length): > char = c_fun_str[i] > - # When char is upper case and no '_' appears before, do more checks > + # When char is upper case and no '_' appears before, > + # do more checks > if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': > if i < length - 1 and c_fun_str[i + 1].islower(): > new_name += '_' The comment paraphrases the if condition. Feels useless. Let's drop it. > @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: > protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' > > :param name: The name to map. > - :param protect: If true, avoid returning certain ticklish identifiers > - (like C keywords) by prepending ``q_``. > + :param protect: If true, avoid returning certain ticklish > + identifiers (like C keywords) by prepending > + ``q_``. Better: :param protect: If true, avoid returning certain ticklish identifiers (like C keywords) by prepending ``q_``. For what it's worth, this indentation style is also used in the Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text like you did, because that wastes screen real estate when the parameter names are long, and tempts people to aligning all the parameters, like :param name: The name to map. :param protect: If true, avoid returning certain ticklish identifiers (like C keywords) by prepending ``q_``. which leads to either churn or inconsistency when parameters with longer names get added. > """ > # ANSI X3J11/88-090, 3.1.1 > c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py > index fee8c671e7..210b56974f 100644 > --- a/scripts/qapi/events.py > +++ b/scripts/qapi/events.py > @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: > """ > Generate a struct variable holding the event parameters. > > - Initialize it with the function arguments defined in `gen_event_send`. > + Initialize it with the function arguments defined in > + `gen_event_send`. > """ > assert not typ.variants > ret = mcgen(''' Looks like a wash. I figure the doc string will be rewritten to Sphinx format (or whatever other format we adopt for our Python code) anyway, so let's not mess with it now. > @@ -86,9 +87,9 @@ def gen_event_send(name: str, > # FIXME: Our declaration of local variables (and of 'errp' in the > # parameter list) can collide with exploded members of the event's > # data type passed in as parameters. If this collision ever hits in > - # practice, we can rename our local variables with a leading _ prefix, > - # or split the code into a wrapper function that creates a boxed > - # 'param' object then calls another to do the real work. > + # practice, we can rename our local variables with a leading _ > + # prefix, or split the code into a wrapper function that creates a > + # boxed 'param' object then calls another to do the real work. > have_args = boxed or (arg_type and not arg_type.is_empty()) > > ret = mcgen(''' Improvement. > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index 1fa503bdbd..c54980074e 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -63,9 +63,9 @@ def _bottom(self) -> str: > return '' > > def write(self, output_dir: str) -> None: > - # Include paths starting with ../ are used to reuse modules of the main > - # schema in specialised schemas. Don't overwrite the files that are > - # already generated for the main schema. > + # Include paths starting with ../ are used to reuse modules > + # of the main schema in specialised schemas. Don't overwrite > + # the files that are already generated for the main schema. > if self.fname.startswith('../'): > return > pathname = os.path.join(output_dir, self.fname) Improvement, but mind PEP 8's "You should use two spaces after a sentence-ending period in multi-sentence comments". > @@ -189,7 +189,7 @@ def _bottom(self) -> str: > @contextmanager > def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: > """ > - A with-statement context manager that wraps with `start_if()` / `end_if()`. > + A context manager that wraps output with `start_if()` / `end_if()`. > > :param ifcond: A sequence of conditionals, passed to `start_if()`. > :param args: any number of `QAPIGenCCode`. Improvement. > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 9a348ca2e5..faf00013ad 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -61,8 +61,9 @@ > # With optional annotations, the type of all values is: > # JSONValue = Union[_Value, Annotated[_Value]] > # > -# Sadly, mypy does not support recursive types; so the _Stub alias is used to > -# mark the imprecision in the type model where we'd otherwise use JSONValue. > +# Sadly, mypy does not support recursive types; so the _Stub alias is > +# used to mark the imprecision in the type model where we'd otherwise > +# use JSONValue. > _Stub = Any > _Scalar = Union[str, bool, None] > _NonScalar = Union[Dict[str, _Stub], List[_Stub]] Improvement. > @@ -217,7 +218,8 @@ def visit_end(self) -> None: > self._name_map = {} > > def visit_needed(self, entity: QAPISchemaEntity) -> bool: > - # Ignore types on first pass; visit_end() will pick up used types > + # Ignore types on first pass; > + # visit_end() will pick up used types Looks a bit odd. Since the original is only slightly over the limit, we can keep it. Alternatively. # Ignore types on first pass; visit_end() will pick up the # types that are actually used > return not isinstance(entity, QAPISchemaType) > > def _name(self, name: str) -> str: > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py > index 703e7ed1ed..5bcac83985 100644 > --- a/scripts/qapi/main.py > +++ b/scripts/qapi/main.py > @@ -1,5 +1,5 @@ > -# This work is licensed under the terms of the GNU GPL, version 2 or later. > -# See the COPYING file in the top-level directory. > +# This work is licensed under the terms of the GNU GPL, version 2 or > +# later. See the COPYING file in the top-level directory. Let's drop this one. The line is only slightly too long, and consistency with the copright notices elsewhere is more important. > > """ > QAPI Generator > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 58267c3db9..d5bf91f2b0 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): > self._indent = indent > > def append(self, line): > - # Strip leading spaces corresponding to the expected indent level > - # Blank lines are always OK. > + # Strip leading spaces corresponding to the expected indent > + # level. Blank lines are always OK. > if line: > indent = re.match(r'\s*', line).end() > if indent < self._indent: Improvement, but mind PEP 8's "You should use two spaces after a sentence-ending period". > @@ -353,10 +353,10 @@ def connect(self, member): > self.member = member > > def __init__(self, parser, info): > - # self._parser is used to report errors with QAPIParseError. The > - # resulting error position depends on the state of the parser. > - # It happens to be the beginning of the comment. More or less > - # servicable, but action at a distance. > + # self._parser is used to report errors with QAPIParseError. > + # The resulting error position depends on the state of the > + # parser. It happens to be the beginning of the comment. More > + # or less servicable, but action at a distance. > self._parser = parser > self.info = info > self.symbol = None Why not. Two spaces again. > @@ -430,7 +430,8 @@ def _append_body_line(self, line): > if not line.endswith(':'): > raise QAPIParseError(self._parser, "line should end with ':'") > self.symbol = line[1:-1] > - # FIXME invalid names other than the empty string aren't flagged > + # FIXME invalid names other than the empty string aren't > + # flagged > if not self.symbol: > raise QAPIParseError(self._parser, "invalid name") > elif self.symbol: Not an improvement, drop the hunk. > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 703b446fd2..01cdd753cd 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: > @classmethod > def is_builtin_module(cls, name: str) -> bool: > """ > - The built-in module is a single System module for the built-in types. > + Return true when given the built-in module name. > > - It is always "./builtin". > + The built-in module is a specific System module for the built-in > + types. It is always "./builtin". > """ > return name == cls.BUILTIN_MODULE_NAME > I figure the doc string will be rewritten to Sphinx format anyway, so let's not mess with it now. > @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): > m.connect_doc(doc) > > def is_implicit(self): > - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() > + # See QAPISchema._make_implicit_enum_type() and > + # ._def_predefineds() > return self.name.endswith('Kind') or self.name == 'QType' > > def c_type(self): Not an improvement, drop the hunk. > @@ -421,9 +423,9 @@ def check(self, schema): > > self.members = members # mark completed > > - # Check that the members of this type do not cause duplicate JSON members, > - # and update seen to track the members seen so far. Report any errors > - # on behalf of info, which is not necessarily self.info > + # Check that the members of this type do not cause duplicate JSON > + # members, and update seen to track the members seen so far. Report > + # any errors on behalf of info, which is not necessarily self.info > def check_clash(self, info, seen): > assert self._checked > assert not self.variants # not implemented Improvement. Two spaces again. > @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): > def check(self, schema): > super().check(schema) > self.variants.tag_member.check(schema) > - # Not calling self.variants.check_clash(), because there's nothing > - # to clash with > + # Not calling self.variants.check_clash(), because there's > + # nothing to clash with > self.variants.check(schema, {}) > - # Alternate branch names have no relation to the tag enum values; > - # so we have to check for potential name collisions ourselves. > + # Alternate branch names have no relation to the tag enum > + # values; so we have to check for potential name collisions > + # ourselves. > seen = {} > types_seen = {} > for v in self.variants.variants: Why not. > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 20d572a23a..2e67ab1752 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -35,8 +35,8 @@ > from .source import QAPISourceInfo > > > -# variants must be emitted before their container; track what has already > -# been output > +# variants must be emitted before their container; track what has > +# already been output > objects_seen = set() > > Why not. > @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: > ''')) > > def visit_begin(self, schema: QAPISchema) -> None: > - # gen_object() is recursive, ensure it doesn't visit the empty type > + # gen_object() is recursive, ensure > + # it doesn't visit the empty type Looks a bit odd. Since the original is only slightly over the limit, we can keep it. Pattern: turning single line comments into multi-line comments to avoid small length overruns is usually not an improvement. > objects_seen.add(schema.the_empty_object_type.name) > > def _gen_type_cleanup(self, name: str) -> None: Bottom line: I find some hunks likable enough. Ways forward: 1. If you need to respin: 1.1. you may keep this patch, and work in my feedback. 1.2. you may drop it. I can pick it up and take care of it. 2. If we decide to go without a respin: 2.1. I can work in my feedback in my tree. 2.2. I can extract the patch and take care of it separately. I'd prefer to avoid 2.1, because I feel it's too much change for comfort. 1.1. vs. 1.2 would be up to you. [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings
On 4/16/21 8:44 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> PEP8's BDFL writes: "For flowing long blocks of text with fewer >> structural restrictions (docstrings or comments), the line length should >> be limited to 72 characters." >> >> I do not like this patch. I have included it explicitly to recommend we >> do not pay any further heed to the 72 column limit. > > Let me go through the patch hunk by hunk to see what I like and what I > don't like. > > In case you'd prefer not to pay any further heed to line length: please > check out my comment on c_name() anyway. It's about doc string style, > and relevant regardless of line length limits. > Right, yeah. I just don't think this is productive right now. I'll read it, though! >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/.flake8 | 1 + >> scripts/qapi/common.py | 8 +++++--- >> scripts/qapi/events.py | 9 +++++---- >> scripts/qapi/gen.py | 8 ++++---- >> scripts/qapi/introspect.py | 8 +++++--- >> scripts/qapi/main.py | 4 ++-- >> scripts/qapi/parser.py | 15 ++++++++------- >> scripts/qapi/schema.py | 23 +++++++++++++---------- >> scripts/qapi/types.py | 7 ++++--- >> 9 files changed, 47 insertions(+), 36 deletions(-) >> >> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 >> index 6b158c68b8..4f00455290 100644 >> --- a/scripts/qapi/.flake8 >> +++ b/scripts/qapi/.flake8 >> @@ -1,2 +1,3 @@ >> [flake8] >> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's >> +max-doc-length = 72 >> \ No newline at end of file > > Since we intend to make use of PEP 8's license to go over the line > length limit, having the build gripe about it is not useful. Drop. > >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index cbd3fd81d3..6e3d9b8ecd 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: >> length = len(c_fun_str) >> for i in range(length): >> char = c_fun_str[i] >> - # When char is upper case and no '_' appears before, do more checks >> + # When char is upper case and no '_' appears before, >> + # do more checks >> if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': >> if i < length - 1 and c_fun_str[i + 1].islower(): >> new_name += '_' > > The comment paraphrases the if condition. Feels useless. Let's drop > it. > >> @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: >> protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >> >> :param name: The name to map. >> - :param protect: If true, avoid returning certain ticklish identifiers >> - (like C keywords) by prepending ``q_``. >> + :param protect: If true, avoid returning certain ticklish >> + identifiers (like C keywords) by prepending >> + ``q_``. > > Better: > > :param protect: If true, avoid returning certain ticklish > identifiers (like C keywords) by prepending ``q_``. > > For what it's worth, this indentation style is also used in the > Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text > like you did, because that wastes screen real estate when the parameter > names are long, and tempts people to aligning all the parameters, like > > :param name: The name to map. > :param protect: If true, avoid returning certain ticklish identifiers > (like C keywords) by prepending ``q_``. > > which leads to either churn or inconsistency when parameters with longer > names get added. > Yeah, that should be fine. I don't like the wasted margin space either, but I suppose I like the "two column" layout for ease of visual distinction of the parameter names. I suppose it isn't really worth the kind of column-reformatting churn and the wasted space. ...And if we do print a sphinx manual for this, I'll get my visual distinction there in the rendered output. I'm fine with adopting this style to cover the entire Python codebase. It will be an eventual thing, though: I think we need to agree on a style guide document and in that same series, fix up the instances of defying that guide. I think it's important to pair that work, because the ease of finding and fixing those style deviations will help inform how pragmatic the style guide is. I feel like it's something I want to do very soon, but not right now. Maybe during the next freeze we can tackle it? >> """ >> # ANSI X3J11/88-090, 3.1.1 >> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', >> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py >> index fee8c671e7..210b56974f 100644 >> --- a/scripts/qapi/events.py >> +++ b/scripts/qapi/events.py >> @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: >> """ >> Generate a struct variable holding the event parameters. >> >> - Initialize it with the function arguments defined in `gen_event_send`. >> + Initialize it with the function arguments defined in >> + `gen_event_send`. >> """ >> assert not typ.variants >> ret = mcgen(''' > > Looks like a wash. I figure the doc string will be rewritten to Sphinx > format (or whatever other format we adopt for our Python code) anyway, > so let's not mess with it now. > >> @@ -86,9 +87,9 @@ def gen_event_send(name: str, >> # FIXME: Our declaration of local variables (and of 'errp' in the >> # parameter list) can collide with exploded members of the event's >> # data type passed in as parameters. If this collision ever hits in >> - # practice, we can rename our local variables with a leading _ prefix, >> - # or split the code into a wrapper function that creates a boxed >> - # 'param' object then calls another to do the real work. >> + # practice, we can rename our local variables with a leading _ >> + # prefix, or split the code into a wrapper function that creates a >> + # boxed 'param' object then calls another to do the real work. >> have_args = boxed or (arg_type and not arg_type.is_empty()) >> >> ret = mcgen(''' > > Improvement. > >> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >> index 1fa503bdbd..c54980074e 100644 >> --- a/scripts/qapi/gen.py >> +++ b/scripts/qapi/gen.py >> @@ -63,9 +63,9 @@ def _bottom(self) -> str: >> return '' >> >> def write(self, output_dir: str) -> None: >> - # Include paths starting with ../ are used to reuse modules of the main >> - # schema in specialised schemas. Don't overwrite the files that are >> - # already generated for the main schema. >> + # Include paths starting with ../ are used to reuse modules >> + # of the main schema in specialised schemas. Don't overwrite >> + # the files that are already generated for the main schema. >> if self.fname.startswith('../'): >> return >> pathname = os.path.join(output_dir, self.fname) > > Improvement, but mind PEP 8's "You should use two spaces after a > sentence-ending period in multi-sentence comments". > How important is this, and why? My existing prejudice is that it's only a superficial detail of writing with no real impact. (Of course, a single space typist WOULD believe that, wouldn't they? Those single-space typists are all the same!) >> @@ -189,7 +189,7 @@ def _bottom(self) -> str: >> @contextmanager >> def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: >> """ >> - A with-statement context manager that wraps with `start_if()` / `end_if()`. >> + A context manager that wraps output with `start_if()` / `end_if()`. >> >> :param ifcond: A sequence of conditionals, passed to `start_if()`. >> :param args: any number of `QAPIGenCCode`. > > Improvement. > >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index 9a348ca2e5..faf00013ad 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -61,8 +61,9 @@ >> # With optional annotations, the type of all values is: >> # JSONValue = Union[_Value, Annotated[_Value]] >> # >> -# Sadly, mypy does not support recursive types; so the _Stub alias is used to >> -# mark the imprecision in the type model where we'd otherwise use JSONValue. >> +# Sadly, mypy does not support recursive types; so the _Stub alias is >> +# used to mark the imprecision in the type model where we'd otherwise >> +# use JSONValue. >> _Stub = Any >> _Scalar = Union[str, bool, None] >> _NonScalar = Union[Dict[str, _Stub], List[_Stub]] > > Improvement. > >> @@ -217,7 +218,8 @@ def visit_end(self) -> None: >> self._name_map = {} >> >> def visit_needed(self, entity: QAPISchemaEntity) -> bool: >> - # Ignore types on first pass; visit_end() will pick up used types >> + # Ignore types on first pass; >> + # visit_end() will pick up used types > > Looks a bit odd. Since the original is only slightly over the limit, we > can keep it. Alternatively. > > # Ignore types on first pass; visit_end() will pick up the > # types that are actually used > >> return not isinstance(entity, QAPISchemaType) >> >> def _name(self, name: str) -> str: >> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py >> index 703e7ed1ed..5bcac83985 100644 >> --- a/scripts/qapi/main.py >> +++ b/scripts/qapi/main.py >> @@ -1,5 +1,5 @@ >> -# This work is licensed under the terms of the GNU GPL, version 2 or later. >> -# See the COPYING file in the top-level directory. >> +# This work is licensed under the terms of the GNU GPL, version 2 or >> +# later. See the COPYING file in the top-level directory. > > Let's drop this one. The line is only slightly too long, and > consistency with the copright notices elsewhere is more important. > >> >> """ >> QAPI Generator >> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >> index 58267c3db9..d5bf91f2b0 100644 >> --- a/scripts/qapi/parser.py >> +++ b/scripts/qapi/parser.py >> @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): >> self._indent = indent >> >> def append(self, line): >> - # Strip leading spaces corresponding to the expected indent level >> - # Blank lines are always OK. >> + # Strip leading spaces corresponding to the expected indent >> + # level. Blank lines are always OK. >> if line: >> indent = re.match(r'\s*', line).end() >> if indent < self._indent: > > Improvement, but mind PEP 8's "You should use two spaces after a > sentence-ending period". > >> @@ -353,10 +353,10 @@ def connect(self, member): >> self.member = member >> >> def __init__(self, parser, info): >> - # self._parser is used to report errors with QAPIParseError. The >> - # resulting error position depends on the state of the parser. >> - # It happens to be the beginning of the comment. More or less >> - # servicable, but action at a distance. >> + # self._parser is used to report errors with QAPIParseError. >> + # The resulting error position depends on the state of the >> + # parser. It happens to be the beginning of the comment. More >> + # or less servicable, but action at a distance. >> self._parser = parser >> self.info = info >> self.symbol = None > > Why not. Two spaces again. > >> @@ -430,7 +430,8 @@ def _append_body_line(self, line): >> if not line.endswith(':'): >> raise QAPIParseError(self._parser, "line should end with ':'") >> self.symbol = line[1:-1] >> - # FIXME invalid names other than the empty string aren't flagged >> + # FIXME invalid names other than the empty string aren't >> + # flagged >> if not self.symbol: >> raise QAPIParseError(self._parser, "invalid name") >> elif self.symbol: > > Not an improvement, drop the hunk. > >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> index 703b446fd2..01cdd753cd 100644 >> --- a/scripts/qapi/schema.py >> +++ b/scripts/qapi/schema.py >> @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: >> @classmethod >> def is_builtin_module(cls, name: str) -> bool: >> """ >> - The built-in module is a single System module for the built-in types. >> + Return true when given the built-in module name. >> >> - It is always "./builtin". >> + The built-in module is a specific System module for the built-in >> + types. It is always "./builtin". >> """ >> return name == cls.BUILTIN_MODULE_NAME >> > > I figure the doc string will be rewritten to Sphinx format anyway, so > let's not mess with it now. > >> @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): >> m.connect_doc(doc) >> >> def is_implicit(self): >> - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() >> + # See QAPISchema._make_implicit_enum_type() and >> + # ._def_predefineds() >> return self.name.endswith('Kind') or self.name == 'QType' >> >> def c_type(self): > > Not an improvement, drop the hunk. > >> @@ -421,9 +423,9 @@ def check(self, schema): >> >> self.members = members # mark completed >> >> - # Check that the members of this type do not cause duplicate JSON members, >> - # and update seen to track the members seen so far. Report any errors >> - # on behalf of info, which is not necessarily self.info >> + # Check that the members of this type do not cause duplicate JSON >> + # members, and update seen to track the members seen so far. Report >> + # any errors on behalf of info, which is not necessarily self.info >> def check_clash(self, info, seen): >> assert self._checked >> assert not self.variants # not implemented > > Improvement. Two spaces again. > >> @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): >> def check(self, schema): >> super().check(schema) >> self.variants.tag_member.check(schema) >> - # Not calling self.variants.check_clash(), because there's nothing >> - # to clash with >> + # Not calling self.variants.check_clash(), because there's >> + # nothing to clash with >> self.variants.check(schema, {}) >> - # Alternate branch names have no relation to the tag enum values; >> - # so we have to check for potential name collisions ourselves. >> + # Alternate branch names have no relation to the tag enum >> + # values; so we have to check for potential name collisions >> + # ourselves. >> seen = {} >> types_seen = {} >> for v in self.variants.variants: > > Why not. > >> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py >> index 20d572a23a..2e67ab1752 100644 >> --- a/scripts/qapi/types.py >> +++ b/scripts/qapi/types.py >> @@ -35,8 +35,8 @@ >> from .source import QAPISourceInfo >> >> >> -# variants must be emitted before their container; track what has already >> -# been output >> +# variants must be emitted before their container; track what has >> +# already been output >> objects_seen = set() >> >> > > Why not. > >> @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: >> ''')) >> >> def visit_begin(self, schema: QAPISchema) -> None: >> - # gen_object() is recursive, ensure it doesn't visit the empty type >> + # gen_object() is recursive, ensure >> + # it doesn't visit the empty type > > Looks a bit odd. Since the original is only slightly over the limit, we > can keep it. > > Pattern: turning single line comments into multi-line comments to avoid > small length overruns is usually not an improvement. > Yep, that's my core argument against turning on the option for flake8. Usually rephrasing is better than re-flowing, but that wasn't always easy either. (I don't like rewriting things to be less terse, I find it unpleasant, sorry!) Unfortunately, omitting it from flake8 means I'll probably also miss cases where I or someone else have gone slightly over the limit for docstrings, and doubt it will be enforced consistently. "Patches welcome" as the old curse goes. >> objects_seen.add(schema.the_empty_object_type.name) >> >> def _gen_type_cleanup(self, name: str) -> None: > > Bottom line: I find some hunks likable enough. > > Ways forward: > > 1. If you need to respin: > > 1.1. you may keep this patch, and work in my feedback. > > 1.2. you may drop it. I can pick it up and take care of it. > This one, please! I have to admit that my appetite for consistency runs out right around here, but I'll never reject someone else doing this kind of work if they find it important. You may also wish to look into the Python packaging series at some point, as you may be able to augment the tests to provide a "manual" run that produces some extra warnings from time to time that you may want to address, which you might find helpful for pursuing these kinds of cleanups in the future where I suspect they will inevitably regress. > 2. If we decide to go without a respin: > > 2.1. I can work in my feedback in my tree. > > 2.2. I can extract the patch and take care of it separately. > > I'd prefer to avoid 2.1, because I feel it's too much change for > comfort. 1.1. vs. 1.2 would be up to you. > > > > [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings >
John Snow <jsnow@redhat.com> writes: > On 4/16/21 8:44 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>> structural restrictions (docstrings or comments), the line length should >>> be limited to 72 characters." >>> >>> I do not like this patch. I have included it explicitly to recommend we >>> do not pay any further heed to the 72 column limit. >> >> Let me go through the patch hunk by hunk to see what I like and what I >> don't like. >> >> In case you'd prefer not to pay any further heed to line length: please >> check out my comment on c_name() anyway. It's about doc string style, >> and relevant regardless of line length limits. >> > > Right, yeah. I just don't think this is productive right now. I'll read > it, though! Thanks! >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/.flake8 | 1 + >>> scripts/qapi/common.py | 8 +++++--- >>> scripts/qapi/events.py | 9 +++++---- >>> scripts/qapi/gen.py | 8 ++++---- >>> scripts/qapi/introspect.py | 8 +++++--- >>> scripts/qapi/main.py | 4 ++-- >>> scripts/qapi/parser.py | 15 ++++++++------- >>> scripts/qapi/schema.py | 23 +++++++++++++---------- >>> scripts/qapi/types.py | 7 ++++--- >>> 9 files changed, 47 insertions(+), 36 deletions(-) >>> >>> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 >>> index 6b158c68b8..4f00455290 100644 >>> --- a/scripts/qapi/.flake8 >>> +++ b/scripts/qapi/.flake8 >>> @@ -1,2 +1,3 @@ >>> [flake8] >>> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's >>> +max-doc-length = 72 >>> \ No newline at end of file >> >> Since we intend to make use of PEP 8's license to go over the line >> length limit, having the build gripe about it is not useful. Drop. >> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index cbd3fd81d3..6e3d9b8ecd 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: >>> length = len(c_fun_str) >>> for i in range(length): >>> char = c_fun_str[i] >>> - # When char is upper case and no '_' appears before, do more checks >>> + # When char is upper case and no '_' appears before, >>> + # do more checks >>> if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': >>> if i < length - 1 and c_fun_str[i + 1].islower(): >>> new_name += '_' >> >> The comment paraphrases the if condition. Feels useless. Let's drop >> it. >> >>> @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: >>> protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >>> >>> :param name: The name to map. >>> - :param protect: If true, avoid returning certain ticklish identifiers >>> - (like C keywords) by prepending ``q_``. >>> + :param protect: If true, avoid returning certain ticklish >>> + identifiers (like C keywords) by prepending >>> + ``q_``. >> >> Better: >> >> :param protect: If true, avoid returning certain ticklish >> identifiers (like C keywords) by prepending ``q_``. >> >> For what it's worth, this indentation style is also used in the >> Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text >> like you did, because that wastes screen real estate when the parameter >> names are long, and tempts people to aligning all the parameters, like >> >> :param name: The name to map. >> :param protect: If true, avoid returning certain ticklish identifiers >> (like C keywords) by prepending ``q_``. >> >> which leads to either churn or inconsistency when parameters with longer >> names get added. >> > > Yeah, that should be fine. I don't like the wasted margin space either, > but I suppose I like the "two column" layout for ease of visual > distinction of the parameter names. I suppose it isn't really worth the > kind of column-reformatting churn and the wasted space. > > ...And if we do print a sphinx manual for this, I'll get my visual > distinction there in the rendered output. I'm fine with adopting this > style to cover the entire Python codebase. > > It will be an eventual thing, though: I think we need to agree on a > style guide document and in that same series, fix up the instances of > defying that guide. I think it's important to pair that work, because > the ease of finding and fixing those style deviations will help inform > how pragmatic the style guide is. Makes sense. The introduction of "sphinxy" doc strings (starting with commit adcb9b36c) may have been premature. > I feel like it's something I want to do very soon, but not right now. > Maybe during the next freeze we can tackle it? Whenever you're ready. Until then, I feel we should try to minimize doc string churn. Leave existing doc strings alone unless they're harmful. Add new ones only when we believe they're helpful enough to justify some churn later. >>> """ >>> # ANSI X3J11/88-090, 3.1.1 >>> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', >>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py >>> index fee8c671e7..210b56974f 100644 >>> --- a/scripts/qapi/events.py >>> +++ b/scripts/qapi/events.py >>> @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: >>> """ >>> Generate a struct variable holding the event parameters. >>> >>> - Initialize it with the function arguments defined in `gen_event_send`. >>> + Initialize it with the function arguments defined in >>> + `gen_event_send`. >>> """ >>> assert not typ.variants >>> ret = mcgen(''' >> >> Looks like a wash. I figure the doc string will be rewritten to Sphinx >> format (or whatever other format we adopt for our Python code) anyway, >> so let's not mess with it now. >> >>> @@ -86,9 +87,9 @@ def gen_event_send(name: str, >>> # FIXME: Our declaration of local variables (and of 'errp' in the >>> # parameter list) can collide with exploded members of the event's >>> # data type passed in as parameters. If this collision ever hits in >>> - # practice, we can rename our local variables with a leading _ prefix, >>> - # or split the code into a wrapper function that creates a boxed >>> - # 'param' object then calls another to do the real work. >>> + # practice, we can rename our local variables with a leading _ >>> + # prefix, or split the code into a wrapper function that creates a >>> + # boxed 'param' object then calls another to do the real work. >>> have_args = boxed or (arg_type and not arg_type.is_empty()) >>> >>> ret = mcgen(''' >> >> Improvement. >> >>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>> index 1fa503bdbd..c54980074e 100644 >>> --- a/scripts/qapi/gen.py >>> +++ b/scripts/qapi/gen.py >>> @@ -63,9 +63,9 @@ def _bottom(self) -> str: >>> return '' >>> >>> def write(self, output_dir: str) -> None: >>> - # Include paths starting with ../ are used to reuse modules of the main >>> - # schema in specialised schemas. Don't overwrite the files that are >>> - # already generated for the main schema. >>> + # Include paths starting with ../ are used to reuse modules >>> + # of the main schema in specialised schemas. Don't overwrite >>> + # the files that are already generated for the main schema. >>> if self.fname.startswith('../'): >>> return >>> pathname = os.path.join(output_dir, self.fname) >> >> Improvement, but mind PEP 8's "You should use two spaces after a >> sentence-ending period in multi-sentence comments". >> > > How important is this, and why? My existing prejudice is that it's only > a superficial detail of writing with no real impact. Holy wars have been fought over less. > (Of course, a single space typist WOULD believe that, wouldn't they? > Those single-space typists are all the same!) I offer three reasons: * Local consistency * Stick to PEP 8 unless you have good reason not to. * It makes Emacs sentence commands work by default. >>> @@ -189,7 +189,7 @@ def _bottom(self) -> str: >>> @contextmanager >>> def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: >>> """ >>> - A with-statement context manager that wraps with `start_if()` / `end_if()`. >>> + A context manager that wraps output with `start_if()` / `end_if()`. >>> >>> :param ifcond: A sequence of conditionals, passed to `start_if()`. >>> :param args: any number of `QAPIGenCCode`. >> >> Improvement. >> >>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >>> index 9a348ca2e5..faf00013ad 100644 >>> --- a/scripts/qapi/introspect.py >>> +++ b/scripts/qapi/introspect.py >>> @@ -61,8 +61,9 @@ >>> # With optional annotations, the type of all values is: >>> # JSONValue = Union[_Value, Annotated[_Value]] >>> # >>> -# Sadly, mypy does not support recursive types; so the _Stub alias is used to >>> -# mark the imprecision in the type model where we'd otherwise use JSONValue. >>> +# Sadly, mypy does not support recursive types; so the _Stub alias is >>> +# used to mark the imprecision in the type model where we'd otherwise >>> +# use JSONValue. >>> _Stub = Any >>> _Scalar = Union[str, bool, None] >>> _NonScalar = Union[Dict[str, _Stub], List[_Stub]] >> >> Improvement. >> >>> @@ -217,7 +218,8 @@ def visit_end(self) -> None: >>> self._name_map = {} >>> >>> def visit_needed(self, entity: QAPISchemaEntity) -> bool: >>> - # Ignore types on first pass; visit_end() will pick up used types >>> + # Ignore types on first pass; >>> + # visit_end() will pick up used types >> >> Looks a bit odd. Since the original is only slightly over the limit, we >> can keep it. Alternatively. >> >> # Ignore types on first pass; visit_end() will pick up the >> # types that are actually used >> >>> return not isinstance(entity, QAPISchemaType) >>> >>> def _name(self, name: str) -> str: >>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py >>> index 703e7ed1ed..5bcac83985 100644 >>> --- a/scripts/qapi/main.py >>> +++ b/scripts/qapi/main.py >>> @@ -1,5 +1,5 @@ >>> -# This work is licensed under the terms of the GNU GPL, version 2 or later. >>> -# See the COPYING file in the top-level directory. >>> +# This work is licensed under the terms of the GNU GPL, version 2 or >>> +# later. See the COPYING file in the top-level directory. >> >> Let's drop this one. The line is only slightly too long, and >> consistency with the copright notices elsewhere is more important. >> >>> >>> """ >>> QAPI Generator >>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>> index 58267c3db9..d5bf91f2b0 100644 >>> --- a/scripts/qapi/parser.py >>> +++ b/scripts/qapi/parser.py >>> @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): >>> self._indent = indent >>> >>> def append(self, line): >>> - # Strip leading spaces corresponding to the expected indent level >>> - # Blank lines are always OK. >>> + # Strip leading spaces corresponding to the expected indent >>> + # level. Blank lines are always OK. >>> if line: >>> indent = re.match(r'\s*', line).end() >>> if indent < self._indent: >> >> Improvement, but mind PEP 8's "You should use two spaces after a >> sentence-ending period". >> >>> @@ -353,10 +353,10 @@ def connect(self, member): >>> self.member = member >>> >>> def __init__(self, parser, info): >>> - # self._parser is used to report errors with QAPIParseError. The >>> - # resulting error position depends on the state of the parser. >>> - # It happens to be the beginning of the comment. More or less >>> - # servicable, but action at a distance. >>> + # self._parser is used to report errors with QAPIParseError. >>> + # The resulting error position depends on the state of the >>> + # parser. It happens to be the beginning of the comment. More >>> + # or less servicable, but action at a distance. >>> self._parser = parser >>> self.info = info >>> self.symbol = None >> >> Why not. Two spaces again. >> >>> @@ -430,7 +430,8 @@ def _append_body_line(self, line): >>> if not line.endswith(':'): >>> raise QAPIParseError(self._parser, "line should end with ':'") >>> self.symbol = line[1:-1] >>> - # FIXME invalid names other than the empty string aren't flagged >>> + # FIXME invalid names other than the empty string aren't >>> + # flagged >>> if not self.symbol: >>> raise QAPIParseError(self._parser, "invalid name") >>> elif self.symbol: >> >> Not an improvement, drop the hunk. >> >>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >>> index 703b446fd2..01cdd753cd 100644 >>> --- a/scripts/qapi/schema.py >>> +++ b/scripts/qapi/schema.py >>> @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: >>> @classmethod >>> def is_builtin_module(cls, name: str) -> bool: >>> """ >>> - The built-in module is a single System module for the built-in types. >>> + Return true when given the built-in module name. >>> >>> - It is always "./builtin". >>> + The built-in module is a specific System module for the built-in >>> + types. It is always "./builtin". >>> """ >>> return name == cls.BUILTIN_MODULE_NAME >>> >> >> I figure the doc string will be rewritten to Sphinx format anyway, so >> let's not mess with it now. >> >>> @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): >>> m.connect_doc(doc) >>> >>> def is_implicit(self): >>> - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() >>> + # See QAPISchema._make_implicit_enum_type() and >>> + # ._def_predefineds() >>> return self.name.endswith('Kind') or self.name == 'QType' >>> >>> def c_type(self): >> >> Not an improvement, drop the hunk. >> >>> @@ -421,9 +423,9 @@ def check(self, schema): >>> >>> self.members = members # mark completed >>> >>> - # Check that the members of this type do not cause duplicate JSON members, >>> - # and update seen to track the members seen so far. Report any errors >>> - # on behalf of info, which is not necessarily self.info >>> + # Check that the members of this type do not cause duplicate JSON >>> + # members, and update seen to track the members seen so far. Report >>> + # any errors on behalf of info, which is not necessarily self.info >>> def check_clash(self, info, seen): >>> assert self._checked >>> assert not self.variants # not implemented >> >> Improvement. Two spaces again. >> >>> @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): >>> def check(self, schema): >>> super().check(schema) >>> self.variants.tag_member.check(schema) >>> - # Not calling self.variants.check_clash(), because there's nothing >>> - # to clash with >>> + # Not calling self.variants.check_clash(), because there's >>> + # nothing to clash with >>> self.variants.check(schema, {}) >>> - # Alternate branch names have no relation to the tag enum values; >>> - # so we have to check for potential name collisions ourselves. >>> + # Alternate branch names have no relation to the tag enum >>> + # values; so we have to check for potential name collisions >>> + # ourselves. >>> seen = {} >>> types_seen = {} >>> for v in self.variants.variants: >> >> Why not. >> >>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py >>> index 20d572a23a..2e67ab1752 100644 >>> --- a/scripts/qapi/types.py >>> +++ b/scripts/qapi/types.py >>> @@ -35,8 +35,8 @@ >>> from .source import QAPISourceInfo >>> >>> >>> -# variants must be emitted before their container; track what has already >>> -# been output >>> +# variants must be emitted before their container; track what has >>> +# already been output >>> objects_seen = set() >>> >>> >> >> Why not. >> >>> @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: >>> ''')) >>> >>> def visit_begin(self, schema: QAPISchema) -> None: >>> - # gen_object() is recursive, ensure it doesn't visit the empty type >>> + # gen_object() is recursive, ensure >>> + # it doesn't visit the empty type >> >> Looks a bit odd. Since the original is only slightly over the limit, we >> can keep it. >> >> Pattern: turning single line comments into multi-line comments to avoid >> small length overruns is usually not an improvement. >> > > Yep, that's my core argument against turning on the option for flake8. > Usually rephrasing is better than re-flowing, but that wasn't always > easy either. (I don't like rewriting things to be less terse, I find it > unpleasant, sorry!) > > Unfortunately, omitting it from flake8 means I'll probably also miss > cases where I or someone else have gone slightly over the limit for > docstrings, and doubt it will be enforced consistently. I'm happy to correct the occasional minor style issue manually. > "Patches welcome" as the old curse goes. > >>> objects_seen.add(schema.the_empty_object_type.name) >>> >>> def _gen_type_cleanup(self, name: str) -> None: >> >> Bottom line: I find some hunks likable enough. >> >> Ways forward: >> >> 1. If you need to respin: >> >> 1.1. you may keep this patch, and work in my feedback. >> >> 1.2. you may drop it. I can pick it up and take care of it. > > This one, please! You got it. > I have to admit that my appetite for consistency runs out right around > here, but I'll never reject someone else doing this kind of work if they > find it important. > > You may also wish to look into the Python packaging series at some > point, as you may be able to augment the tests to provide a "manual" run > that produces some extra warnings from time to time that you may want to > address, which you might find helpful for pursuing these kinds of > cleanups in the future where I suspect they will inevitably regress. Good idea. We may find other warnings we don't want to treat as errors, but do want to consider case by case. >> 2. If we decide to go without a respin: >> >> 2.1. I can work in my feedback in my tree. >> >> 2.2. I can extract the patch and take care of it separately. >> >> I'd prefer to avoid 2.1, because I feel it's too much change for >> comfort. 1.1. vs. 1.2 would be up to you. >> >> >> >> [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings >>
On 4/17/21 6:52 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: >> On 4/16/21 8:44 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >> It will be an eventual thing, though: I think we need to agree on a >> style guide document and in that same series, fix up the instances of >> defying that guide. I think it's important to pair that work, because >> the ease of finding and fixing those style deviations will help inform >> how pragmatic the style guide is. > > Makes sense. > > The introduction of "sphinxy" doc strings (starting with commit > adcb9b36c) may have been premature. > Somewhat premature, but what other format is there? It would have been worse to adopt Numpy or google style. We'll dial it in over time, it will be fine. >> I feel like it's something I want to do very soon, but not right now. >> Maybe during the next freeze we can tackle it? > > Whenever you're ready. > > Until then, I feel we should try to minimize doc string churn. Leave > existing doc strings alone unless they're harmful. Add new ones only > when we believe they're helpful enough to justify some churn later. > OK. After the expr comments, I actually didn't add very many. I think I add one or two for the parser because I had trouble understanding at a glance how it worked, but most of the tiny functions and helpers I left alone. I barely touched schema.py, because it was complex and I had some visions of refactoring it a little to make some of the typing better later. >>> Improvement, but mind PEP 8's "You should use two spaces after a >>> sentence-ending period in multi-sentence comments". >>> >> >> How important is this, and why? My existing prejudice is that it's only >> a superficial detail of writing with no real impact. > > Holy wars have been fought over less. > :) >> (Of course, a single space typist WOULD believe that, wouldn't they? >> Those single-space typists are all the same!) > > I offer three reasons: > > * Local consistency > > * Stick to PEP 8 unless you have good reason not to. > > * It makes Emacs sentence commands work by default. > For me, it's another thing in the category of "I don't actually mind either way", and can foresee myself accepting a patch using either style without comment. Inconsistency here doesn't really bother me unless it's inconsistent within a single docstring. For QAPI, since you're the maintainer, I can adhere to your style. For the purposes of all Python code, though, I am not sure I want to bother enforcing it myself. You're always welcome to post-edit anything I've written for typographical consistency as you see fit, I genuinely won't mind. (It saves me the trouble of having to copy-edit for something I am visually blind to.) That said, I'll try to match your preferred style for QAPI at a minimum. I notice that emacs' reflow command does not always insert two spaces if a paragraph already sneaks in under the column limit; is there a way to *force* it to add two spaces? >> Unfortunately, omitting it from flake8 means I'll probably also miss >> cases where I or someone else have gone slightly over the limit for >> docstrings, and doubt it will be enforced consistently. > > I'm happy to correct the occasional minor style issue manually. > If you accept that burden then I have no leg to stand on, I suppose :) >>> 1.2. you may drop it. I can pick it up and take care of it. >> >> This one, please! > > You got it. > Thanks! You can do that whenever, it won't interfere with anything in the interim. --js
diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 index 6b158c68b8..4f00455290 100644 --- a/scripts/qapi/.flake8 +++ b/scripts/qapi/.flake8 @@ -1,2 +1,3 @@ [flake8] extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's +max-doc-length = 72 \ No newline at end of file diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index cbd3fd81d3..6e3d9b8ecd 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: length = len(c_fun_str) for i in range(length): char = c_fun_str[i] - # When char is upper case and no '_' appears before, do more checks + # When char is upper case and no '_' appears before, + # do more checks if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': if i < length - 1 and c_fun_str[i + 1].islower(): new_name += '_' @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' :param name: The name to map. - :param protect: If true, avoid returning certain ticklish identifiers - (like C keywords) by prepending ``q_``. + :param protect: If true, avoid returning certain ticklish + identifiers (like C keywords) by prepending + ``q_``. """ # ANSI X3J11/88-090, 3.1.1 c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index fee8c671e7..210b56974f 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: """ Generate a struct variable holding the event parameters. - Initialize it with the function arguments defined in `gen_event_send`. + Initialize it with the function arguments defined in + `gen_event_send`. """ assert not typ.variants ret = mcgen(''' @@ -86,9 +87,9 @@ def gen_event_send(name: str, # FIXME: Our declaration of local variables (and of 'errp' in the # parameter list) can collide with exploded members of the event's # data type passed in as parameters. If this collision ever hits in - # practice, we can rename our local variables with a leading _ prefix, - # or split the code into a wrapper function that creates a boxed - # 'param' object then calls another to do the real work. + # practice, we can rename our local variables with a leading _ + # prefix, or split the code into a wrapper function that creates a + # boxed 'param' object then calls another to do the real work. have_args = boxed or (arg_type and not arg_type.is_empty()) ret = mcgen(''' diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 1fa503bdbd..c54980074e 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -63,9 +63,9 @@ def _bottom(self) -> str: return '' def write(self, output_dir: str) -> None: - # Include paths starting with ../ are used to reuse modules of the main - # schema in specialised schemas. Don't overwrite the files that are - # already generated for the main schema. + # Include paths starting with ../ are used to reuse modules + # of the main schema in specialised schemas. Don't overwrite + # the files that are already generated for the main schema. if self.fname.startswith('../'): return pathname = os.path.join(output_dir, self.fname) @@ -189,7 +189,7 @@ def _bottom(self) -> str: @contextmanager def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: """ - A with-statement context manager that wraps with `start_if()` / `end_if()`. + A context manager that wraps output with `start_if()` / `end_if()`. :param ifcond: A sequence of conditionals, passed to `start_if()`. :param args: any number of `QAPIGenCCode`. diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 9a348ca2e5..faf00013ad 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -61,8 +61,9 @@ # With optional annotations, the type of all values is: # JSONValue = Union[_Value, Annotated[_Value]] # -# Sadly, mypy does not support recursive types; so the _Stub alias is used to -# mark the imprecision in the type model where we'd otherwise use JSONValue. +# Sadly, mypy does not support recursive types; so the _Stub alias is +# used to mark the imprecision in the type model where we'd otherwise +# use JSONValue. _Stub = Any _Scalar = Union[str, bool, None] _NonScalar = Union[Dict[str, _Stub], List[_Stub]] @@ -217,7 +218,8 @@ def visit_end(self) -> None: self._name_map = {} def visit_needed(self, entity: QAPISchemaEntity) -> bool: - # Ignore types on first pass; visit_end() will pick up used types + # Ignore types on first pass; + # visit_end() will pick up used types return not isinstance(entity, QAPISchemaType) def _name(self, name: str) -> str: diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 703e7ed1ed..5bcac83985 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -1,5 +1,5 @@ -# This work is licensed under the terms of the GNU GPL, version 2 or later. -# See the COPYING file in the top-level directory. +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. """ QAPI Generator diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 58267c3db9..d5bf91f2b0 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): self._indent = indent def append(self, line): - # Strip leading spaces corresponding to the expected indent level - # Blank lines are always OK. + # Strip leading spaces corresponding to the expected indent + # level. Blank lines are always OK. if line: indent = re.match(r'\s*', line).end() if indent < self._indent: @@ -353,10 +353,10 @@ def connect(self, member): self.member = member def __init__(self, parser, info): - # self._parser is used to report errors with QAPIParseError. The - # resulting error position depends on the state of the parser. - # It happens to be the beginning of the comment. More or less - # servicable, but action at a distance. + # self._parser is used to report errors with QAPIParseError. + # The resulting error position depends on the state of the + # parser. It happens to be the beginning of the comment. More + # or less servicable, but action at a distance. self._parser = parser self.info = info self.symbol = None @@ -430,7 +430,8 @@ def _append_body_line(self, line): if not line.endswith(':'): raise QAPIParseError(self._parser, "line should end with ':'") self.symbol = line[1:-1] - # FIXME invalid names other than the empty string aren't flagged + # FIXME invalid names other than the empty string aren't + # flagged if not self.symbol: raise QAPIParseError(self._parser, "invalid name") elif self.symbol: diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 703b446fd2..01cdd753cd 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: @classmethod def is_builtin_module(cls, name: str) -> bool: """ - The built-in module is a single System module for the built-in types. + Return true when given the built-in module name. - It is always "./builtin". + The built-in module is a specific System module for the built-in + types. It is always "./builtin". """ return name == cls.BUILTIN_MODULE_NAME @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): m.connect_doc(doc) def is_implicit(self): - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() + # See QAPISchema._make_implicit_enum_type() and + # ._def_predefineds() return self.name.endswith('Kind') or self.name == 'QType' def c_type(self): @@ -421,9 +423,9 @@ def check(self, schema): self.members = members # mark completed - # Check that the members of this type do not cause duplicate JSON members, - # and update seen to track the members seen so far. Report any errors - # on behalf of info, which is not necessarily self.info + # Check that the members of this type do not cause duplicate JSON + # members, and update seen to track the members seen so far. Report + # any errors on behalf of info, which is not necessarily self.info def check_clash(self, info, seen): assert self._checked assert not self.variants # not implemented @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): def check(self, schema): super().check(schema) self.variants.tag_member.check(schema) - # Not calling self.variants.check_clash(), because there's nothing - # to clash with + # Not calling self.variants.check_clash(), because there's + # nothing to clash with self.variants.check(schema, {}) - # Alternate branch names have no relation to the tag enum values; - # so we have to check for potential name collisions ourselves. + # Alternate branch names have no relation to the tag enum + # values; so we have to check for potential name collisions + # ourselves. seen = {} types_seen = {} for v in self.variants.variants: diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 20d572a23a..2e67ab1752 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -35,8 +35,8 @@ from .source import QAPISourceInfo -# variants must be emitted before their container; track what has already -# been output +# variants must be emitted before their container; track what has +# already been output objects_seen = set() @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: ''')) def visit_begin(self, schema: QAPISchema) -> None: - # gen_object() is recursive, ensure it doesn't visit the empty type + # gen_object() is recursive, ensure + # it doesn't visit the empty type objects_seen.add(schema.the_empty_object_type.name) def _gen_type_cleanup(self, name: str) -> None:
PEP8's BDFL writes: "For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters." I do not like this patch. I have included it explicitly to recommend we do not pay any further heed to the 72 column limit. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/.flake8 | 1 + scripts/qapi/common.py | 8 +++++--- scripts/qapi/events.py | 9 +++++---- scripts/qapi/gen.py | 8 ++++---- scripts/qapi/introspect.py | 8 +++++--- scripts/qapi/main.py | 4 ++-- scripts/qapi/parser.py | 15 ++++++++------- scripts/qapi/schema.py | 23 +++++++++++++---------- scripts/qapi/types.py | 7 ++++--- 9 files changed, 47 insertions(+), 36 deletions(-)