Skip to content

pylint: disable=all comment is applied to the whole file #8725

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

Open
ruro opened this issue May 29, 2023 · 5 comments
Open

pylint: disable=all comment is applied to the whole file #8725

ruro opened this issue May 29, 2023 · 5 comments
Labels
C: Pragma's Needs decision 🔒 Needs a decision before implemention or rejection
Milestone

Comments

@ruro
Copy link
Contributor

ruro commented May 29, 2023

Bug description

Consider the following example:

"""Module level docstring"""

def enable_here():
    """This function should have a warning."""
    x = "bad name"
    return x


def disable_here():
    """No pylint warnings in this function, please."""
    # pylint: disable=all
    x = "bad name"
    return x

For some reason, the disable=all comment disables the warnings for the whole file, instead of only for the current block/current line/next line/etc. Compare this with the exact same file, but with disable=invalid-name instead of disable=all.

Configuration

None

Command used

pylint test.py

Pylint output

(no output)

Expected output

************* Module test
test.py:5:4: C0103: Variable name "x" doesn't conform to snake_case naming style (invalid-name)

Pylint version

pylint 2.17.4
astroid 2.15.5
Python 3.10.10 (main, Mar  5 2023, 22:26:53) [GCC 12.2.1 20230201]

OS / Environment

Manjaro (Arch Linux)

Additional dependencies

astroid==2.15.5
dill==0.3.6
isort==5.12.0
lazy-object-proxy==1.9.0
mccabe==0.7.0
platformdirs==3.5.1
pylint==2.17.4
tomli==2.0.1
tomlkit==0.11.8
typing_extensions==4.6.2
wrapt==1.15.0
@ruro ruro added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 29, 2023
@mbyrnepr2
Copy link
Member

This seems like expected behaviour.
# pylint: disable=all is deprecated and the new way is to use # pylint: skip-file (the wording of the latter way makes it clear why the checks are disabled for the entire file).

The deprecated-pragma warning is disabled by default:

pylint --enable=deprecated-pragma example.py
example.py:11:0: I0022: Pragma "disable=all" is deprecated, use "skip-file" instead (deprecated-pragma)

deprecated-pragma previously was known as deprecated-disable-all.

@mbyrnepr2 mbyrnepr2 closed this as not planned Won't fix, can't repro, duplicate, stale May 31, 2023
@mbyrnepr2 mbyrnepr2 added Won't fix/not planned and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 31, 2023
@ruro
Copy link
Contributor Author

ruro commented Jun 1, 2023

@mbyrnepr2 I think, that the current behaviour is still broken. Either disable=all should work as expected, or it should warn about it being deprecated (by default).

The fact that it's a synonym (?) for skip-file is not obvious to the user. For example, if I see such a disable=all marker during core review, I would naturally assume that it only silences the warnings current line/block.

@mbyrnepr2 mbyrnepr2 reopened this Jun 1, 2023
@mbyrnepr2 mbyrnepr2 added C: Pragma's Needs decision 🔒 Needs a decision before implemention or rejection labels Jun 1, 2023
@mbyrnepr2
Copy link
Member

Yeah I don't know why deprecated-pragma would be disabled by default :)
Let's open a merge-request and see if anyone has an opinion on changing this or leaving it alone.

mbyrnepr2 added a commit to mbyrnepr2/pylint that referenced this issue Jun 1, 2023
@mbyrnepr2 mbyrnepr2 modified the milestone: 2.17.5 Jun 1, 2023
@mbyrnepr2
Copy link
Member

Ok we have a reason not to do this. Please see this comment regarding a wider design-change around these pragmas.

@mbyrnepr2 mbyrnepr2 added Won't fix/not planned and removed Discussion 🤔 Needs decision 🔒 Needs a decision before implemention or rejection labels Jun 1, 2023
@mbyrnepr2 mbyrnepr2 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 1, 2023
mbyrnepr2 added a commit to mbyrnepr2/pylint that referenced this issue Jun 6, 2023
@jacobtylerwalls
Copy link
Member

Hi Mark. Thanks for digging into the relevant context here. I didn't know about any of this.

Please see #8731 (comment) regarding a wider design-change around these pragmas.

I wonder if that's an orthogonal issue. Pierre wants to wait if our idea is to make the deprecation more visible, but that's a big if. What if we just fix the behavior to not be surprising and forget about a deprecation that no one has ever seen?

The fact that it's a synonym (?) for skip-file is not obvious to the user. For example, if I see such a disable=all marker during core review, I would naturally assume that it only silences the warnings current line/block.

👍

Reopening to continue the discussion about whether to "undo" the deprecation and fix it.

@jacobtylerwalls jacobtylerwalls added Needs decision 🔒 Needs a decision before implemention or rejection and removed Won't fix/not planned labels Jul 4, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.0.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Pragma's Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants