Allow per-function skips #203

larsoner · 2016-08-30T20:00:35Z

Allows per-function skips. Ready for review/merge from my end.

I added tests and updated docs in hopefully correct ways.

Closes #202.
Closes #97.

Comments like "#noqa" can be added to the end of function and method definitions to skip checks.

sigmavirus24 · 2016-08-31T13:05:20Z

I'm starting to think that certain things should be part of reusable library code because now pycodestyle, pydocstyle, and flake8 have just slightly different implementations of almost the same thing and that's going to lead to really weird inconsistencies between the three.

Further, from what I can tell, this will directly conflict with people running pydocstyle from inside Flake8 (e.g., via flake8-docstrings), which is something I'm strongly against.

larsoner · 2016-08-31T13:41:23Z

Further, from what I can tell, this will directly conflict with people running pydocstyle from inside Flake8 (e.g., via flake8-docstrings), which is something I'm strongly against.

How do you mean? # noqa will turn off both flake8 and pydocstyle, whereas doing something like # pydocstyle: D101 should only affect pydocstyle, and in theory something like # pydocstyle: D101 flake8: E204 should work fine too.

larsoner · 2016-08-31T13:42:41Z

I'm starting to think that certain things should be part of reusable library code because now pycodestyle, pydocstyle, and flake8 have just slightly different implementations of almost the same thing and that's going to lead to really weird inconsistencies between the three.

This might be true, but I'd prefer to do that as a separate subsequent refactoring. It will be hard to get all these libraries to settle on and adopt a new shared API, and in the meantime, something like this is badly needed for this library to be usable for many people.

larsoner · 2016-08-31T14:20:17Z

Actually it looks like flake8 offers:

# noqa to ignore everything (and really this probably should cause pydocstyle to ignore it, too)
# noqa: E501 so we can easily piggyback on that with e.g. # noqa: E501,D101 in a compatible way. I tested this in flake8 and it works fine.
# flake8: noqa we can again piggyback with # flake8: noqa pydocstyle:noqa in a compatible way. I tested this with flake8 and it works fine.

So I don't see any conflicts in this scheme...?

sigmavirus24 · 2016-08-31T15:16:50Z

docs/release_notes.rst

+* Added the ability to ignore specific function and method doctstrings with
+  comments:
+
+    1. "# noqa" or "# pydocstyle: noqa" skips all checks.


@Eric89GXL This and # pydocstyle: D102,D203 are what I'm talking about. You should just use # noqa below. Even so, having # pydocstyle: noqa in a file will skip the whole file? That's what people are going to expect from having used # flake8: noqa. You're going to confuse people with this.

No it won't skip the whole file, just the line in question. I'll make it do what flake8 does

sigmavirus24 · 2016-08-31T15:19:28Z

This might be true, but I'd prefer to do that as a separate subsequent refactoring.

Absolutely, but that means the logic in this PR would only complicate that because it's completely turning the semantics of these comments on their head. I don't see a need to use # pycodestyle: <codes> as the prefix here. pydocstyle and flake8 have an overlap in developers. Reusing # noqa: <codes> seems reasonable. It will also keep comments succinct.

larsoner · 2016-08-31T17:44:21Z

@sigmavirus24 Got rid of # pydocstyle: noqa and it should be fully compatible with flake8 now, feel free to see if you're happy with it

jayvdb · 2016-08-31T19:30:44Z

Do you really need to invoke pydocstyle directly, and add noqa support into pydocstyle ?

Could you use flake8-docstrings which gives you # noqa or also add flake8-putty to give you # flake8: disable=<codes> (but is customisable to support # noqa: <codes>) ?

larsoner · 2016-08-31T19:32:00Z

I'll try those alternatives

larsoner · 2016-09-01T00:50:30Z

@jayvdb I tried pip install flake8-docstrings and then doing flake8 instead of pydocstyle. It does respect # noqa and # noqa: D103, which helps. But at least at a first pass it has some drawbacks out of the box:

It doesn't respect the pydocstyle set of skips/includes in setup.cfg. So I'm not sure how to have flake8 run on one set of files for style and another set of files for pyflakes and pep8 without having to pass arguments to each (cumbersome and defeats purpose of config).
Installing it this way makes flake8 by default always do pydocstyle checks, which is undesirable. I assume there is a way to have it "off" by default (?), but this again adds barrier to entry with every additional step necessary to configure their system.

I assume there are workarounds for this, but isn't it less clean than having pydocstyle respect # noqa and # noqa: <codes> when invoked on its own? After all, isn't pydocstyle designed to be run on its own? The documentation makes it seem that way. With this PR, I can just tell people "run pip install flake8 pydocstyle; flake8; pydocstyle" and everything will work properly, and it doesn't seem so obvious with the flake8 case. But it seems like there is sustained opposition to having this functionality here, so I'll drop it unless you're enthusiastic about it.

jayvdb · 2016-09-01T02:46:42Z

flake8-putty solves 1.
You can disable pydocstyle by default, using ignore=D, but again you probably wont need this if you use flake8-putty to decide what parts of the code that pydocstyle should be used or ignored.

sigmavirus24 · 2016-09-01T10:45:05Z

@Eric89GXL note, that there are many teams in the PyCQA and @jayvdb is a member of one (which is not the team that maintains pydocstyle).

My opposition was merely in having divergent behaviour between two projects which you seem to have resolved.

If you have need for specific configuration from pydocstyle in flake8's configuration through flake8-docstrings you can create an issue on https://gitlab.com/pycqa/flake8-docstrings and we can work towards fixing those deficiencies you're finding.

larsoner · 2016-09-01T10:59:15Z

But maybe I'm missing how simple your alternative proposal is. Can you write the docs to describe the process you mean, since they should probably be included anyway?

larsoner · 2016-09-01T10:59:15Z

So in other words, once I have flake8-docstyle installed, I have to change existing workflows? This seems inferior to having pydocstyle support reasonable options out of the box without needing to install two or three other libraries and change calls and configurations in non obvious ways. I'm not sure why you'd want to promote this as the standard for how to achieve this functionality. Why such strong opposition to this? Maintenance burden?

larsoner · 2016-09-01T11:52:43Z

So this PR's documentation / API proposal was:

``pydocstyle`` inline commenting to skip specific checks on specific
functions or methods. The supported comments that can be added are:

1. ``"# noqa"`` skips all checks.

2. ``"# noqa: D102,D203"`` can be used to skip specific checks. Note that
   this is compatible with skips from `flake8 <http://flake8.pycqa.org/>`_,
   e.g. ``# noqa: D102,E501,D203``.

Since it's not being used, I guess the doc should have something like this added to show people how to use it with current master:

3. Install `flake8-docstrings`

4. Change any existing `flake8` calls on your system have to have `--ignore=D`, as installation of `flake8-docstrings` will enable these new checks by default.

5. Install `flake8-putty`.

6. Configure `flake8-putty` for each repo to triage which files get passed to `flake8` checking and `pydocstyle` checking, by <doing something>

I think step (4) is especially painful for devs who work on multiple repos, many of which use flake8 (I know many such devs). It discourages actually using pydocstyle. It's also odd that users need to go through all this to get one type of skip functionality, when other types (like config types) are supported directly by pydocstyle out of the box.

Nurdok · 2016-09-01T11:56:17Z

Hi folks - sorry for joining this party so late :)

@sigmavirus24 - it seems like the issues you mentioned have been resolved (not using pydocstyle in the ignore comment). As long as this issue doesn't break flake8, it seems fine to me. If we can come up with a nice way to share the code for the ignoring mechanism later, I'll happily refactor this.

@Eric89GXL - thanks for the PR. I'll go ahead and give you a code review now :)

Nurdok · 2016-09-01T11:57:17Z

docs/usage.rst

@@ -17,3 +17,9 @@ Configuration Files
 ^^^^^^^^^^^^^^^^^^^

 .. include:: snippets/config.rst
+
+
+Per-file configuration


How about "In-file configuration" so it'll be more clear?

larsoner · 2016-09-01T16:35:37Z

Thanks fro the comments @Nurdok, comments addressed. I'm happy to squash to a single commit so I avoid touching a bunch of now-unchanged lines if it helps.

Nurdok · 2016-09-01T17:46:15Z

docs/usage.rst

+In-file configuration
+^^^^^^^^^^^^^^^^^^^^^
+
+.. include:: snippets/per_file.rst


The snippet name should also be changed.

Nurdok · 2016-09-01T17:46:56Z

src/pydocstyle.py

@@ -140,13 +144,17 @@ def is_empty_or_comment(line):
        return ''.join(reversed(list(filtered_src)))

    def __str__(self):
-        return 'in %s %s `%s`' % (self._publicity, self._human, self.name)
+        out = 'in %s %s `%s`' % (self._publicity, self._human, self.name)


Please change this to format as well while you're at it :)

Should I just replace all instances, or might that create unnecessary merge conflicts with other open PRs?

I'll only hold you to lines you changed, but if you feel like going over the code, I'd be grateful.

Nurdok · 2016-09-01T17:48:50Z

@Eric89GXL Just a few more comments :) I don't really mind about squashing either way.

larsoner · 2016-09-02T16:52:34Z

@Nurdok comments addressed. The only % string operations left in the file are to logging calls, which AFAIK have to stay as %s-style.

Nurdok · 2016-09-02T19:34:51Z

Merged!
@Eric89GXL - thank you so much for putting in the time and effort and especially in the face of resistence.

larsoner · 2016-09-02T19:37:17Z

No problem, thanks for the helpful discussions.

@Nurdok the next one that would make my life easier is #171. Any thoughts over there?

Allow per-function skips

7d3f471

Comments like "#noqa" can be added to the end of function and method definitions to skip checks.

larsoner mentioned this pull request Aug 30, 2016

MRG: Update docstyle mne-tools/mne-python#3549

Merged

sigmavirus24 reviewed Aug 31, 2016
View reviewed changes

Fix types of skips

5b69acd

larsoner mentioned this pull request Aug 31, 2016

Check __doc__ assignments #185

Open

larsoner closed this Aug 31, 2016

Nurdok reviewed Sep 1, 2016
View reviewed changes

Cleaner triage based on error code

98623fe

larsoner reopened this Sep 1, 2016

Nurdok reviewed Sep 1, 2016
View reviewed changes

docs/usage.rst

In-file configuration

^^^^^^^^^^^^^^^^^^^^^

.. include:: snippets/per_file.rst

Copy link

Member

Nurdok Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snippet name should also be changed.

Nurdok reviewed Sep 1, 2016
View reviewed changes

larsoner added 2 commits September 2, 2016 12:50

Remove % in str for .format

4d43201

Move doc file to proper name

dbaec42

Nurdok merged commit 2ce9e41 into PyCQA:master Sep 2, 2016

larsoner deleted the skips branch September 2, 2016 19:34

Nurdok mentioned this pull request Sep 6, 2016

Refactor - split to modules, add parser tests, update readme #200

Merged

kavdev mentioned this pull request Sep 12, 2016

Implement pydocstyle to enforce documentation dj-stripe/dj-stripe#340

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow per-function skips #203

Allow per-function skips #203

larsoner commented Aug 30, 2016 •

edited

Loading

sigmavirus24 commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Aug 31, 2016

sigmavirus24 Aug 31, 2016

larsoner Aug 31, 2016 via email

sigmavirus24 commented Aug 31, 2016

larsoner commented Aug 31, 2016

jayvdb commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Sep 1, 2016

jayvdb commented Sep 1, 2016

sigmavirus24 commented Sep 1, 2016

larsoner commented Sep 1, 2016 via email

larsoner commented Sep 1, 2016 via email

larsoner commented Sep 1, 2016

Nurdok commented Sep 1, 2016

Nurdok Sep 1, 2016

larsoner commented Sep 1, 2016

Nurdok Sep 1, 2016

Nurdok Sep 1, 2016

larsoner Sep 1, 2016 via email

Nurdok Sep 1, 2016

Nurdok commented Sep 1, 2016

larsoner commented Sep 2, 2016

Nurdok commented Sep 2, 2016

larsoner commented Sep 2, 2016

Allow per-function skips #203

Allow per-function skips #203

Conversation

larsoner commented Aug 30, 2016 • edited Loading

sigmavirus24 commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Aug 31, 2016

sigmavirus24 Aug 31, 2016

Choose a reason for hiding this comment

larsoner Aug 31, 2016 via email

Choose a reason for hiding this comment

sigmavirus24 commented Aug 31, 2016

larsoner commented Aug 31, 2016

jayvdb commented Aug 31, 2016

larsoner commented Aug 31, 2016

larsoner commented Sep 1, 2016

jayvdb commented Sep 1, 2016

sigmavirus24 commented Sep 1, 2016

larsoner commented Sep 1, 2016 via email

larsoner commented Sep 1, 2016 via email

larsoner commented Sep 1, 2016

Nurdok commented Sep 1, 2016

Nurdok Sep 1, 2016

Choose a reason for hiding this comment

larsoner commented Sep 1, 2016

Nurdok Sep 1, 2016

Choose a reason for hiding this comment

Nurdok Sep 1, 2016

Choose a reason for hiding this comment

larsoner Sep 1, 2016 via email

Choose a reason for hiding this comment

Nurdok Sep 1, 2016

Choose a reason for hiding this comment

Nurdok commented Sep 1, 2016

larsoner commented Sep 2, 2016

Nurdok commented Sep 2, 2016

larsoner commented Sep 2, 2016

larsoner commented Aug 30, 2016 •

edited

Loading