Skip to content
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

Changes to match new class docstring standard of PEP257 #91

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
5 participants
@fdion
Copy link

commented Jan 7, 2015

This is to match Guido's change here:
https://hg.python.org/peps/rev/9b715d8246db
and the reason is in a comment by Guido here (first comment at bottom):

http://raspberry-python.blogspot.com/2015/01/significant-pep257-change.html

fdion added some commits Jan 7, 2015

PEP257 changed class docstring to be assymetrical
The earlier behaviour was to have a blank line before and after a class docstring. New behaviour defined here: https://www.python.org/dev/peps/pep-0257/

"Insert a blank line after all docstrings (one-line or multi-line) that document a class -- generally speaking, the class's methods are separated from each other by a single blank line, and the docstring needs to be offset from the first method by a blank line."
@sigmavirus24

This comment has been minimized.

Copy link
Member

commented Jan 8, 2015

Might be good to squash those two middle commits since they have near identical commit messages. Otherwise, 👍

@lvh

This comment has been minimized.

Copy link

commented Jan 8, 2015

Big fan of this PR :)

@xZise

This comment has been minimized.

Copy link

commented Jan 8, 2015

Is it maybe possible to still allow 0 or 1 line above the class docstring? Otherwise this would require pywikibot to update all classes and remove the blank line between the class and docstring.

@sigmavirus24

This comment has been minimized.

Copy link
Member

commented Jan 8, 2015

@xZise has a point. If this is to be accepted in its current condition, a major version bump should be planned so users understand that a breaking change (as far as expected output is concerned) will be happening.

@fdion

This comment has been minimized.

Copy link
Author

commented Jan 8, 2015

On pywikibot you are already using --ignore on flake8 (w/flake8-docstrings):
[testenv:flake8]
commands = flake8 --ignore=D102,D103,E122,E127,E241 {posargs}

So you could simply add D203 to this list. I don't think the new wording of pep257 supports 0 or 1 blank lines. Might also be worth the effort to add blank line removal to @myint docformatter tool.

@lvh

This comment has been minimized.

Copy link

commented Jan 8, 2015

Yeah, pyformat should also be made aware of this.

@lvh

This comment has been minimized.

Copy link

commented Jan 8, 2015

Perhaps a default list of ignores? And then either setting --ignore means you have to specify it. That'd be the simplest option. Otherwise --ignore=+D123,-D201 or something? (Maybe with implicit pluses.) But that gets very confusing quickly; I think the default ignore list isn't the worst idea :)

@lvh lvh referenced this pull request Jan 8, 2015

Merged

Linter improvements #877

@sigmavirus24

This comment has been minimized.

Copy link
Member

commented Jan 8, 2015

I think we're trending towards a good solution. I think a default ignore set is good. I also think changing errors associated with codes is bad. Error codes should be immutable. So,

I think this PR should be blocked until a new error code is covered to only check for an empty line after a class-level docstring. The error code that currently covers 1 before and 1 after should be turned off by adding it to the default ignore set. An error code should be added that covers 0 empty lines between a class declaration and the class docstring. That should also be off by default.

Action items:

  • Revert this work
  • Add two new error codes
    • One code for missing empty line after docstring
    • One code for extra empty line(s) before docstring
  • Add default ignore list
    • Add existing (now incorrect) error code to default ignore list
    • Add error code for extra emtpy line(s) before docstring to default ignore list

That's just my opinion though.

@xZise

This comment has been minimized.

Copy link

commented Jan 8, 2015

Yeah it needs to be a new error code, because if you 'reuse' D203 there is no distinction between 'requiring one line above' or 'requiring no line above' and it would disable the test completely. Thinking about it there should be at least to modes: Require 0 or 1 line above and require 0 lines above. Optionally (but I don't need it, just for completeness) require 1 line above. But if it's possible to allow 0 or 1 line above we could wait for fixing our code but already new code could comply.

Maybe error codes aren't the best idea but options, because currently the error codes make sense but with two or three options on how D203 should work. Although maybe D213 and D223 for the different modes of D2x3.

And regarding that we ignore certain: Yes for now, look into the flake8-docstrings-mandatory env. We are trying to build towards that configuration (which is ignoring none).

@Nurdok

This comment has been minimized.

Copy link
Member

commented Jan 9, 2015

The current relevant error codes are:

  • D203: 1 blank required before class docstring.
  • D204: 1 blank required after class docstring.

The guideline for pep257.py is that the PEP257 spec should be the default behavior. However, it's possible to allow checks that aren't part of the spec, so long as they are turned on by default. Furthermore, it is best not to meddle with existing error codes as to not create confusion between versions.

Therefore, this is what should logically happen:

  • D204 is still fine, and should remain as-is, and turned on by default.
  • D203 is now not apart of the PEP257 specification. The error associated with this code should not change, but it should be turned off by default.
  • There should be a new check (the next available code in the category is D210) that checks that there are no blank lines before a class docstring. It should be turned on by default.

Now, there are several ways to implement the default errors feature, while allowing the user to change what is being checked in a sane way:

  1. Change the default value for the --ignore flag to be D203 (and any other error we'll want to ignore in the future). The problem with this is that if the user wants to ignore some error while still ignoring all the default ones, they will have to be specified explicitly.
  2. Add a flag for enabling slienced erros. It could be something like --enable=D203 or something more specific like --enable-d203 or --old-style-class-docstring or something of this nature.
  3. In addition to (2), Add a flag for adding to the ignore list without overriding it. Something like --add-ignore=D203,D401.

I personally tend towards (1), because it's what pep8 does. However it can become less comfortable if there will be many defaultly ignored errors. I'm also wondering how we'll implement flags that check different conventions (like --numpy) and still maintain the --ignore flag functionality.

@xZise

This comment has been minimized.

Copy link

commented Jan 9, 2015

Oh sorry, I thought D203 was about both. Then most of my comment is not valid. Regarding --old-style-class-docstring is it maybe possible to have something like --style-revision. The 'current' would be 0 and the next 1 (or if there is already one scheme that could be used). If now --style-revision 1 is set it'd require 0 and --style-revision 0 would require 1 line between. This obviously doesn't solve if you want both (except if you want “any” by deactivating D203).

Another idea would be to define how many lines D203 allows, so something like --lines-d203=1 would be current state, --lines-d203=0 would be the new default and --lines-d203=0,1 would allow both (this option is obviously ignored if D203 is ignored all together). I'm not sure how flexible it needs to be. Just comma separated ints or if you are using argparse using nargs='+'? Or allow also statements like 0,2-5, 0,2- or -4,6 (the last one is maybe ambiguous because if the leading - so probably only allow 0-4,6). I personally think that comma separated/nargs='+' does suffice.

@Nurdok

This comment has been minimized.

Copy link
Member

commented Jan 20, 2015

I opened a different issue for deciding how we're going to handle ignore lists, default error codes, different convention styles, etc. Please take a look and give your feedback at #96.

@Nurdok

This comment has been minimized.

Copy link
Member

commented Aug 1, 2015

@fdion, issue #96 is now implemented. Do you intend to refactor your PR?

@xZise

This comment has been minimized.

Copy link

commented Aug 1, 2015

As mentioned in #96 I don't really see how that changes the main problem with this PR, that there needs to be a sensible way to still allow the old way, allow both (for the conversion) or just the new way.

@Nurdok

This comment has been minimized.

Copy link
Member

commented Oct 2, 2015

This is implemented in #137.

@Nurdok Nurdok closed this Oct 2, 2015

@Smirl Smirl referenced this pull request Dec 1, 2015

Closed

Update pep257 to 0.7.0 #406

@jayvdb jayvdb referenced this pull request Dec 6, 2015

Merged

Update docstrings to PEP 257 #9

jwhitlock added a commit to mdn/browsercompat that referenced this pull request Jan 20, 2016

bug 1238597 - Upgrade flake8-docstrings
When upgrading from 0.2.1.post1 to 0.2.4, warning D203 gets re-enabled.
This is the warning about "1 blank line required before class
docstring", that was removed from PEP257 by Guido:

PyCQA/pydocstyle#91
http://raspberry-python.blogspot.com/2015/01/significant-pep257-change.html

It is replaced by D211 "No blank lines allowed before class docstring",
but must be re-enabled because we're setting the ignore value in
setup.cfg.

jwhitlock added a commit to mdn/browsercompat that referenced this pull request Jan 20, 2016

bug 1238597 - Upgrade flake8-docstrings
When upgrading from 0.2.1.post1 to 0.2.4, warning D203 gets re-enabled.
This is the warning about "1 blank line required before class
docstring", that was removed from PEP257 by Guido:

PyCQA/pydocstyle#91
http://raspberry-python.blogspot.com/2015/01/significant-pep257-change.html

It is replaced by D211 "No blank lines allowed before class docstring",
but must be re-enabled because we're setting the ignore value in
setup.cfg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.