New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for W505 - doc line length #674
Conversation
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 |
Does W504 need to be in the default ignore list? If the default line length is always |
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. |
I redid this so it's absolutely disabled by default, trying to follow behavior changes similar to On a related note, it seems like using |
Just a heads up, error code W504 conflicts with the same code used in #502 |
Thanks. I assume it doesn't matter if W505 turns up even if W504 is missing? |
Yep that would be fine! |
Any thoughts on how we can make progress on this PR? |
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. |
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) |
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:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/intro.rst
Outdated
@@ -402,6 +404,8 @@ This is the current list of error and warning codes: | |||
+------------+----------------------------------------------------------------------+ | |||
| W503 (*) | line break occurred before a binary operator | | |||
+------------+----------------------------------------------------------------------+ | |||
| W504 (\*^) | doc line too long (82 > 79 characters) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure you don't need the \
.. Edit: though maybe I'm wrong looking up above..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this should be an E503
instead of W504
, since this isn't really do do with "line breaks".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
setup.cfg
Outdated
@@ -5,3 +5,4 @@ universal = 1 | |||
select = | |||
ignore = E226,E24 | |||
max_line_length = 79 | |||
max_doc_length = 79 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be 72 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the tox.ini, I seem to recall setting this to 72 will cause it to fail on the current pycodestyle codebase.
tox.ini
Outdated
@@ -10,9 +10,9 @@ skip_missing_interpreters=True | |||
[testenv] | |||
commands = | |||
{envpython} setup.py install | |||
{envpython} pycodestyle.py --testsuite testsuite | |||
{envpython} pycodestyle.py --max-doc-length=79 --testsuite testsuite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably also be 72, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I change this to 72, I need to actually fix a bunch of docstrings in pycodestyle itself (I can do that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is the right way to go, that we should eat our own dog food, as it were. :)
@brianv0 so is the plan to fix the entire package to be PEP8 docstring length compliant? |
If that is the plan, and it just needs to get done, I can volunteer to help :) |
I'll get back to this this week.. working a bit on it now. |
Hi @IanLee1521 : I added a few more commits (and fixed a few bugs I found |
@brianv0 is this ready for review again? @IanLee1521 are you happy? |
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 |
Who gets to decide on the number to use for codes? I have no idea who the release managers for pycodestyle are. |
#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. |
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. |
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. |
@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. |
Any thoughts? |
6be2f4c
to
6ef3190
Compare
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 All other behavior is still as described. |
Ian's feedback has been resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thank you for merging this. We are really looking forward to using it and it will make our lives much easier. |
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!)