pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.31k stars 1.14k forks source link

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

Open RuRo opened 1 year ago

RuRo commented 1 year ago

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
mbyrnepr2 commented 1 year ago

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.

RuRo commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

jacobtylerwalls commented 1 year ago

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

Please see https://github.com/pylint-dev/pylint/pull/8731#issuecomment-1572582489 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.