Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[Feature Request] Add API to disable noqa support #484

Closed
plinss opened this issue Jun 8, 2020 · 6 comments · Fixed by #485
Closed

[Feature Request] Add API to disable noqa support #484

plinss opened this issue Jun 8, 2020 · 6 comments · Fixed by #485

Comments

@plinss
Copy link
Contributor

plinss commented Jun 8, 2020

flake8 has an option to disable # noqa comments. When running pydocstyle as a flake8 plugin (via flake8-docstrings) and using flake8's --disable-noqa option, # noqa comments still block all violation reports from pydocstyle because pydocstyle is processing those comments itself and deciding to not send the reports at all.

It would be great if pydocstyle supported a feature equivalent to flake8's --disable-noqa and once that's in place, flake8-docstrings activated that feature so that flake8 sees all reports regardless of # noqa comments and could make its own decision to report them or not.

(I understand that would be a separate issue on flake8-docstrings once support is added to pydocstyle, I just mention it to ensure that the feature can be activated directly in ConventionChecker.check_source() as well as via traditional means).

@sigmavirus24
Copy link
Member

Trying to hack this into flake8-docstrings will go poorly. As a Flake8 and Flake8-docstrings maintainer I'd rather start with something in the API only that allows us to gather all results because it will be very confusing going forward why we're overriding --disable-noqa for pydocstyle and that's not the good kind of maintenance burden.

@plinss
Copy link
Contributor Author

plinss commented Jun 8, 2020

I never proposed to hack something into flake8-docstrings to change pydocstyle's behavior, that would be painful to maintain, and it would be trying to solve the problem at the wrong layer. I did file this issue on pydocstyle, not flake8-docstrings.

I'm asking for feature parity between pydocstyle and flake8, they both parse and respect # noqa comments but only flake8 has the option to disable them (if pydocstyle has that ability I'm not finding it, happy to be proven wrong).

pydocstyle not having an equivalent to --disable-noqa (via any mechanism) is preventing flake8's --disable-noqa feature from working with pydocstyle violations. Getting that to work is my ultimate goal here, but pydocstyle getting a --disable-noqa option (or equivalent) for stand-alone use as well seemed reasonable to me.

If you'd rather expose this functionality in pydocstyle via an API only, or prefer some different way of getting violations from pydocstyle to flake8 that bypasses its # noqa handling that'd by fine by me.

@plinss
Copy link
Contributor Author

plinss commented Jun 8, 2020

To me it looks like adding a boolean argument (with default value matching current behavior, of course) to ConventionChecker.check_source and using that to optionally disable the error.code not in definition.skipped_error_codes test on line 124/125 would be the minimally invasive fix.

e.g.

--- src/pydocstyle/checker.py
+++ src/pydocstyle/checker.py
@@ -104,7 +104,7 @@ class ConventionChecker:
         ".+"            # Followed by 1 or more characters - which is the docstring for the parameter
     )
 
-    def check_source(self, source, filename, ignore_decorators=None):
+    def check_source(self, source, filename, ignore_decorators=None, all_errors=False):
         module = parse(StringIO(source), filename)
         for definition in module:
             for this_check in self.checks:
@@ -114,15 +114,15 @@ class ConventionChecker:
                     decorator_skip = ignore_decorators is not None and any(
                         len(ignore_decorators.findall(dec.name)) > 0
                         for dec in definition.decorators)
-                    if not skipping_all and not decorator_skip:
+                    if all_errors or (not skipping_all and not decorator_skip):
                         error = this_check(self, definition,
                                            definition.docstring)
                     else:
                         error = None
                     errors = error if hasattr(error, '__iter__') else [error]
                     for error in errors:
-                        if error is not None and error.code not in \
-                                definition.skipped_error_codes:
+                        if error is not None and (all_errors or error.code not in \
+                                definition.skipped_error_codes):
                             partition = this_check.__doc__.partition('.\n')
                             message, _, explanation = partition
                             error.set_context(explanation=explanation,

(deliberately didn't reformat to keep the diff small, and edited the diff as I missed a test the first time. I haven't actually tested any of this yet, I'm just questioning if this is the direction you're proposing)

flake8-docstyle could then simply add , all_errors=True to it's call to get all errors and let flake8's # noqa handling take over from there.

Instead of a bool it could also be a list of codes to always return if you want more flexibility.

@sigmavirus24
Copy link
Member

flake8-docstyle could then simply add , all_errors=True to it's call to get all errors and let flake8's # noqa handling take over from there.

This is why I preferred this direction. flake8-docstrings really wants to give all of the errors to Flake8 and let that make decisions.

I think it's an anti-pattern for every tool to implement the same options as Flake8 and proliferate that maintenance across a bunch of already stretched thin maintainers. That's why I reacted to your solution-y feature request of adding more maintenance and support burden to this project

plinss added a commit to plinss/pydocstyle that referenced this issue Jun 9, 2020
Added to ConventionChecker.check_source() and ConventionChecker.check().
This allows other systems that integrate pydocstyle,
such as flake8-docstyle, to request errors that are otherwise
skipped due to # noqa comments.

Added test for skip_errors feature in general and with this flag.

Fixes PyCQA#484
@plinss
Copy link
Contributor Author

plinss commented Jun 9, 2020

Apologies, I misstated my original ask. My primary concern was simply to enable this functionality for flake8-docstrings, which is why I explicitly wrote:

I just mention it to ensure that the feature can be activated directly in ConventionChecker.check_source()

It just seemed to me that asking for API surface without exposing that to users was a bit odd, so I phrased it in the terms of a user-facing feature. Especially considering that apparently # noqa support was added to pydocstyle for feature parity with flake8, and it's not quite equivalent.

I'll modify the title of this issue to reflect just the API changes, and submit a separate issue to add the option to expose the API to users. Feel free to close that one as a "won't fix", but I think it's worth having the paper trail of the discussion in case it comes up again or others want to chime in.

In general, I agree that having # noqa processing in plugins to flake8 is an anti-pattern (and one I've seen in a few). My original thought was to request that it simply be dropped from pydocstyle, but I accept that pydocstyle is a stand-alone tool and people use it without flake8. So either internal # noqa processing or requiring some kind of wrapper to post-process the errors would be desired. This approach seems to serve both use cases.

While writing the tests, I noticed that pydocstyle also doesn't respect the flake8 # noqa feature of ignoring errors via prefix, e.g. # noqa: D4. I'll file a separate issue for that, again feel free to close as "won't fix".

@plinss plinss changed the title [Feature Request] Add --disable-noqa support [Feature Request] Add API to disable noqa support Jun 9, 2020
@plinss
Copy link
Contributor Author

plinss commented Jun 9, 2020

(and apologies if you feel I'm spamming you with irrelevant issues. I'll gladly close them myself if you'd prefer.)

sambhav added a commit that referenced this issue Aug 29, 2020
* Add disable_skip_errors flag to ConventionChecker API

Added to ConventionChecker.check_source() and ConventionChecker.check().
This allows other systems that integrate pydocstyle,
such as flake8-docstyle, to request errors that are otherwise
skipped due to # noqa comments.

Added test for skip_errors feature in general and with this flag.

Fixes #484

* Update release notes

* rename argument to ignore inline noqa comments

* Add docstring description of new argument

* Move release note

Co-authored-by: Sambhav Kothari <sambhavs.email@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants