QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
	Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH v4 02/19] flake8: Enforce shorter line length for comments and docstrings
Date: Fri, 26 Mar 2021 12:30:37 -0400
Message-ID: <33866c5d-1eab-739c-f2e8-a35f4a0b0f55@redhat.com> (raw)
In-Reply-To: <871rc277mm.fsf@dusky.pond.sub.org>

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.



  reply index

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25  6:03 [PATCH v4 00/19] qapi: static typing conversion, pt3 John Snow
2021-03-25  6:03 ` [PATCH v4 01/19] qapi/expr: Comment cleanup John Snow
2021-03-25 15:41   ` Markus Armbruster
2021-03-25 20:06     ` John Snow
2021-03-25  6:03 ` [PATCH v4 02/19] flake8: Enforce shorter line length for comments and docstrings John Snow
2021-03-25 15:21   ` Markus Armbruster
2021-03-25 20:20     ` John Snow
2021-03-26  6:26       ` Markus Armbruster
2021-03-26 16:30         ` John Snow [this message]
2021-03-26 16:44           ` Peter Maydell
2021-04-08  8:32             ` Markus Armbruster
2021-04-08  8:58             ` Daniel P. Berrangé
2021-04-09  9:33               ` Markus Armbruster
2021-04-09 17:08                 ` John Snow
2021-04-08  8:35           ` Markus Armbruster
2021-04-16 12:44   ` Markus Armbruster
2021-04-16 20:25     ` John Snow
2021-04-17 10:52       ` Markus Armbruster
2021-04-20 18:06         ` John Snow
2021-03-25  6:03 ` [PATCH v4 03/19] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
2021-03-25  6:03 ` [PATCH v4 04/19] qapi/expr.py: Check for dict instead of OrderedDict John Snow
2021-03-25  6:03 ` [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types John Snow
2021-03-25 14:04   ` Markus Armbruster
2021-03-25 20:48     ` John Snow
2021-03-26  5:40       ` Markus Armbruster
2021-03-26 17:12         ` John Snow
2021-03-25  6:03 ` [PATCH v4 06/19] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
2021-03-25  6:03 ` [PATCH v4 07/19] qapi/expr.py: move string check upwards in check_type John Snow
2021-03-25  6:03 ` [PATCH v4 08/19] qapi: add tests for invalid 'data' field type John Snow
2021-03-25 14:24   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 09/19] qapi/expr.py: Check type of 'data' member John Snow
2021-03-25 14:26   ` Markus Armbruster
2021-03-25 21:04     ` John Snow
2021-03-25  6:03 ` [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases John Snow
2021-03-25 14:33   ` Markus Armbruster
2021-03-25 23:32     ` John Snow
2021-03-25  6:03 ` [PATCH v4 11/19] qapi/expr.py: Modify check_keys to accept any Collection John Snow
2021-03-25 14:45   ` Markus Armbruster
2021-03-25 23:37     ` John Snow
2021-03-25  6:03 ` [PATCH v4 12/19] qapi/expr.py: add type hint annotations John Snow
2021-03-25  6:03 ` [PATCH v4 13/19] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
2021-03-25 15:15   ` Markus Armbruster
2021-03-26  0:07     ` John Snow
2021-03-25  6:03 ` [PATCH v4 14/19] qapi/expr.py: Remove single-letter variable John Snow
2021-03-25  6:03 ` [PATCH v4 15/19] qapi/expr.py: enable pylint checks John Snow
2021-03-25  6:03 ` [PATCH v4 16/19] qapi/expr.py: Add docstrings John Snow
2021-04-14 15:04   ` Markus Armbruster
2021-04-17  1:00     ` John Snow
2021-04-17 13:18       ` Markus Armbruster
2021-04-21  1:27         ` John Snow
2021-04-21 13:58           ` Markus Armbruster
2021-04-21 18:20             ` John Snow
2021-03-25  6:03 ` [PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for static data John Snow
2021-03-25 15:19   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 18/19] qapi/expr.py: move related checks inside check_xxx functions John Snow
2021-03-25  6:03 ` [PATCH v4 19/19] qapi/expr.py: Use an expression checker dispatch table John Snow
2021-03-25 15:46 ` [PATCH v4 00/19] qapi: static typing conversion, pt3 Markus Armbruster
2021-03-26  0:40 ` John Snow
2021-03-26 18:01 ` John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=33866c5d-1eab-739c-f2e8-a35f4a0b0f55@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=michael.roth@amd.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git