Support for W505 - doc line length by brianv0 · Pull Request #674 · PyCQA/pycodestyle

brianv0 · 2017-08-15T19:04:12Z

An implementation of #344.

This implementation takes the effective default of 79 characters for the default max doc length despite PEP8 recommendations being 72, as well as setting only a warning. In addition, W504 is added to the default ignore list.

Builds tested with py27 and py35 (don't have others on my platform, sorry!)

brianv0 · 2017-08-15T19:08:44Z

Just a note: I'm not sure how to best handle the defaults in lines 1607-1609, as well as how to communicate those defaults sensibly to the user.

Instead of saying the default is MAX_LINE_LENGTH (= 79), can we say the default is max-line-length, so it's clear that, if max-line-length is specified, that max-doc-length take that default if it's also not specified.

timj · 2017-08-15T19:14:28Z

Does W504 need to be in the default ignore list? If the default line length is always max-line-length how can W504 ever appear for people? Why shouldn't it be an enabled warning? Do you get two warnings if a line is longer than max-line-length and max-doc-length?

brianv0 · 2017-08-15T19:16:40Z

If you were explicitly ignoring E501 (I imagine there's a large amount of code which does this), then W504 might unexpectedly show up if you upgraded to a new version of pycodestyle and you had long docstrings which were previously ignored.

brianv0 · 2017-08-15T22:01:26Z

I redid this so it's absolutely disabled by default, trying to follow behavior changes similar to --hanging-indent, but it's enabled in tox and the testsuite.

On a related note, it seems like using --testsuite ignores config in setup.cfg.

AWhetter · 2017-08-22T19:31:44Z

Just a heads up, error code W504 conflicts with the same code used in #502

timj · 2017-08-22T19:34:08Z

Thanks. I assume it doesn't matter if W505 turns up even if W504 is missing?

AWhetter · 2017-08-23T00:28:41Z

Yep that would be fine!

timj · 2017-09-05T05:07:12Z

Any thoughts on how we can make progress on this PR?

brianv0 · 2017-10-25T18:01:03Z

Okay, so now this branch is conflicting because it hasn't merged.

I'm happy to fix those conflicts, but it'd be great if we could get some guidance on this.

IanLee1521 · 2017-10-26T01:59:01Z

Hi @brianv0 -- I'm getting myself caught up after some time away... Long story short, I am open to this PR, we would need to resolve the conflicts, but also a comment. In your code, you mix 72 and 79 characters as the line length... for docstrings we would want 72 characters max, right? (ref: https://www.python.org/dev/peps/pep-0008/#maximum-line-length)

brianv0 · 2017-10-26T13:38:22Z

Yes, the goal is to support 72 character line length docstrings and 79 character line-length by default, but allow developers to expand to 99 characters, which the linked-to section mentions:

For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters

Notably, in that case, you are changing the nominal line length (79-99), but not changing the docstring doc length, which is 72. For my purposes, this pull request is also intended to allow that combination (99 character line length + 72 character docstrings), but it's sufficiently general that users could choose the combinations they care about, and it's turned off by default.

IanLee1521

Looks pretty reasonable to me. Once we resolve the conflicts I can review the meat of the function, but it looks like it's on the right track.

IanLee1521 · 2017-10-26T15:01:53Z

 +------------+----------------------------------------------------------------------+
 | W503 (*) | line break occurred before a binary operator |
 +------------+----------------------------------------------------------------------+
+| W504 (\*^) | doc line too long (82 > 79 characters) |


I'm pretty sure you don't need the \ .. Edit: though maybe I'm wrong looking up above..

I'm wondering if this should be an E503 instead of W504, since this isn't really do do with "line breaks".

Should it be an error rather than a warning though? I was trying to be a bit gentler to existing code. We could rename the heading of the W5 section to just say "Line Length" to match the E5 section

IanLee1521 · 2017-10-26T15:07:16Z

 select =
 ignore = E226,E24
 max_line_length = 79
+max_doc_length = 79


shouldn't this be 72 ?

Similar to the tox.ini, I seem to recall setting this to 72 will cause it to fail on the current pycodestyle codebase.

IanLee1521 · 2017-10-26T15:08:30Z

 commands =
 	{envpython} setup.py install
-	{envpython} pycodestyle.py --testsuite testsuite
+	{envpython} pycodestyle.py --max-doc-length=79 --testsuite testsuite


These should probably also be 72, no?

If I change this to 72, I need to actually fix a bunch of docstrings in pycodestyle itself (I can do that)

I think that is the right way to go, that we should eat our own dog food, as it were. :)

timj · 2017-11-29T15:45:57Z

@brianv0 so is the plan to fix the entire package to be PEP8 docstring length compliant?

hoylemd · 2017-11-29T16:33:24Z

If that is the plan, and it just needs to get done, I can volunteer to help :)

brianv0 · 2017-11-29T19:26:41Z

I'll get back to this this week.. working a bit on it now.

brianv0 · 2017-12-01T17:59:28Z

Hi @IanLee1521 : I added a few more commits (and fixed a few bugs I found
b8a2b13) In the docstrings, some of the first lines broke into two lines, and I rewrote a few where I thought the meaning was preserved, and a few more I added a # noqa where it seemed like the docstrings were necessarily long

timj · 2018-01-04T16:44:32Z

@brianv0 is this ready for review again? @IanLee1521 are you happy?

peterjc · 2018-01-11T11:17:06Z

Is the plan to rename this (documentation line length check) to W505 given W503 and W504 would make a nice pair for the binary operator line breaks (and the apparent consensus that W504 would be used for that, see #502)?

(Edit: Fixed leading digits)

Cross reference https://gitlab.com/pycqa/flake8/issues/397

timj · 2018-01-11T16:32:43Z

Who gets to decide on the number to use for codes? I have no idea who the release managers for pycodestyle are.

timj · 2018-01-11T16:33:39Z

#502 seems to have been open for more than a year and a half so I'm not sure whether we can rely on it being merged.

brianv0 · 2018-01-11T18:50:34Z

I'd be happy to come up with a new number if #502 is merged before this Pull Request, but I think it would need a rebase first in any case, as it doesn't pass checks/conflicts with the base branch. I'm not sure using an out-of-order number makes sense until we know which pull requests are to be merged.

sigmavirus24 · 2018-01-18T04:47:30Z

I strongly suspect #502 will be merged prior to this one. I think there is still contention around whether or not this check belongs in pycodestyle at all.

timj · 2018-01-18T17:17:28Z

@brianv0 seems like you should change the code. @sigmavirus24 where can we have the debate over whether this check should be included? I thought we'd already got agreement.

timj · 2018-03-13T04:21:41Z

Any thoughts?

brianv0 · 2018-05-11T16:47:34Z

I've rebased/rewritten this PR to refer to a new error code W505 since support for W504 was merged, and reordered this PR to be two separate commits, one for the implementation of W505 and one so that the code base is self-consistent with the W505 implementation itself (limiting docstring length to 72 characters as PEP8 recommends). I also added support to ignore #! docstrings on the first line as well.

All other behavior is still as described.

Ian's feedback has been resolved

sigmavirus24

This looks great. I'm far more comfortable with this being in the default ignore as well meaning that we won't surprise users. Thank you so much for your diligence and patience.

timj · 2018-05-14T16:57:49Z

Thank you for merging this. We are really looking forward to using it and it will make our lives much easier.

timj mentioned this pull request Sep 21, 2017
Enforce 72 character docstring line width #344
Closed

IanLee1521 previously requested changes Oct 26, 2017
View reviewed changes

tjanez mentioned this pull request Dec 15, 2017
Add a way to pass secrets to processes in a controlled fashion genialis/resolwe#406
Merged

timj mentioned this pull request Mar 13, 2018
DM-10448: Rewrap to 79 characters lsst/utils#35
Closed

brianv0 force-pushed the docstring_support branch from 5d2832b to 6be2f4cCompare May 8, 2018 16:38

brianv0 added 2 commits May 11, 2018 09:35

Implement W505 - Max Doc Length check
f6139a8

Pydoc fixes to support W505 in self-checks
6ef3190

brianv0 force-pushed the docstring_support branch from 6be2f4c to 6ef3190Compare May 11, 2018 16:41

brianv0 changed the title ~~Support for W504 - doc line length~~Support for W505 - doc line lengthMay 11, 2018

sigmavirus24 approved these changes May 14, 2018
View reviewed changes

sigmavirus24 merged commit f4f6e55 into PyCQA:masterMay 14, 2018

brianv0 mentioned this pull request May 25, 2018
Black should have an opinion about doc strings psf/black#144
Closed

peterjc mentioned this pull request Jun 29, 2018
Checks for docstring line lengths PyCQA/pydocstyle#228
Closed

asottile mentioned this pull request Jan 23, 2019
New release? #828
Closed

asottile mentioned this pull request Apr 3, 2021
Cannot use pyflakes to enforce line length *only in docstrings* PyCQA/flake8#232
Closed

Uh oh!

Conversation

brianv0 commented Aug 15, 2017

Uh oh!

brianv0 commented Aug 15, 2017

Uh oh!

timj commented Aug 15, 2017

Uh oh!

brianv0 commented Aug 15, 2017

Uh oh!

brianv0 commented Aug 15, 2017

Uh oh!

AWhetter commented Aug 22, 2017

Uh oh!

timj commented Aug 22, 2017

Uh oh!

AWhetter commented Aug 23, 2017

Uh oh!

timj commented Sep 5, 2017

Uh oh!

brianv0 commented Oct 25, 2017

Uh oh!

IanLee1521 commented Oct 26, 2017

Uh oh!

brianv0 commented Oct 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanLee1521 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timj commented Nov 29, 2017

Uh oh!

hoylemd commented Nov 29, 2017

Uh oh!

brianv0 commented Nov 29, 2017

Uh oh!

brianv0 commented Dec 1, 2017

Uh oh!

timj commented Jan 4, 2018

Uh oh!

peterjc commented Jan 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timj commented Jan 11, 2018

Uh oh!

timj commented Jan 11, 2018

Uh oh!

brianv0 commented Jan 11, 2018

Uh oh!

sigmavirus24 commented Jan 18, 2018

Uh oh!

timj commented Jan 18, 2018

Uh oh!

timj commented Mar 13, 2018

Uh oh!

brianv0 commented May 11, 2018

Uh oh!

sigmavirus24 left a comment

Choose a reason for hiding this comment

Uh oh!

timj commented May 14, 2018

Uh oh!

Reviewers

Assignees

brianv0 commented Oct 26, 2017•
edited
Loading

peterjc commented Jan 11, 2018•
edited
Loading