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

lint-fixme comments not respected #405

Open
samueljsb opened this issue Nov 2, 2023 · 3 comments · Fixed by #441
Open

lint-fixme comments not respected #405

samueljsb opened this issue Nov 2, 2023 · 3 comments · Fixed by #441

Comments

@samueljsb
Copy link
Contributor

samueljsb commented Nov 2, 2023

lint-fixme comments on nodes in some cases are not respected.

comprehensions

In the following file (t.py), we would expect the violation to be reported for the first function, but not for either of the other two.

def f(l):
    return [
        i for i in l
        if isinstance(i, int) or isinstance(i, float)
    ]


def g(l):
    return [
        i for i in l
        # lint-fixme: CollapseIsinstanceChecks
        if isinstance(i, int) or isinstance(i, float)
    ]


def h(l):
    return [
        i for i in l
        if isinstance(i, int) or isinstance(i, float)  # lint-fixme: CollapseIsinstanceChecks
    ]

However, the violation is reported for all three:

$ fixit lint t.py
t.py@4:11 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
t.py@12:11 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
t.py@19:11 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
🛠️  1 file checked, 1 file with errors, 3 auto-fixes available 🛠️

lambdas

In the following file (t.py), we would expect the violation to be reported for the first list, but not for either of the other two.

operators = [
    int,
    str,
    lambda x: float(x),
]

operators = [
    int,
    str,
    # lint-fixme: NoRedundantLambda
    lambda x: float(x),
]

operators = [
    int,
    str,
    lambda x: float(x),  # lint-fixme: NoRedundantLambda
]

However, the violation is reported for all three:

$ fixit lint t.py
t.py@4:4 NoRedundantLambda: The lambda that is wrapping float is redundant. It can unwrapped safely and used purely. (has autofix)
t.py@11:4 NoRedundantLambda: The lambda that is wrapping float is redundant. It can unwrapped safely and used purely. (has autofix)
t.py@17:4 NoRedundantLambda: The lambda that is wrapping float is redundant. It can unwrapped safely and used purely. (has autofix)
🛠️  1 file checked, 1 file with errors, 3 auto-fixes available 🛠️

EDIT: the commit is respected if it is placed before the list:

# lint-fixme: NoRedundantLambda
operators = [
    int,
    str,
    lambda x: float(x),
]
$ fixit lint t.py
🧼 1 file clean 🧼

This is not expected, though, and means that multiple lines will be ignored rather than just the desired one.

methods

In the following file (t.py), we would expect the violation to be reported for the first class, but not for either of the other two.

class foo:
    @classmethod
    def nm(self, a, b, c):
        pass

class foo:
    @classmethod
    # lint-fixme: UseClsInClassmethod
    def nm(self, a, b, c):
        pass

class foo:
    @classmethod
    def nm(self, a, b, c):  # lint-fixme: UseClsInClassmethod
        pass

However, the violation is correctly silenced for the last case, but shown for the middle case that should be silenced:

$ fixit lint t.py
t.py@3:4 UseClsInClassmethod: When using @classmethod, the first argument must be `cls`. (has autofix)
t.py@9:4 UseClsInClassmethod: When using @classmethod, the first argument must be `cls`. (has autofix)
🛠️  1 file checked, 1 file with errors, 2 auto-fixes available 🛠️

supporting information

$ python --version
Python 3.11.5

$ fixit --version
fixit, version 2.1.0

I have also observed this behaviour on Python 3.10.11.

@samueljsb samueljsb changed the title lint-fixme comments ignored inside list comprehensions lint-fixme comments not respected Nov 2, 2023
@samueljsb
Copy link
Contributor Author

I've found another one that's causing us problems: decorators.

from dataclasses import dataclass


@dataclass()
class C:
    pass


# lint-fixme: ExplicitFrozenDataclass
@dataclass()
class C:
    pass


@dataclass()  # lint-fixme: ExplicitFrozenDataclass
class C:
    pass

We'd expect the first function to be reported, and the other two to be silenced.

t.py@4:0 ExplicitFrozenDataclass: When using dataclasses, explicitly specify a frozen keyword argument. Example: `@dataclass(frozen=True)` or `@dataclass(frozen=False)`. Docs: https://docs.python.org/3/library/dataclasses.html (has autofix)
t.py@10:0 ExplicitFrozenDataclass: When using dataclasses, explicitly specify a frozen keyword argument. Example: `@dataclass(frozen=True)` or `@dataclass(frozen=False)`. Docs: https://docs.python.org/3/library/dataclasses.html (has autofix)
🛠️  1 file checked, 1 file with errors, 2 auto-fixes available 🛠️

But we see the first two reported. The last one is correctly silenced.

@samueljsb
Copy link
Contributor Author

I have written some failing tests to demonstrate these problems in #413. I haven't worked out how to demonstrate the function def problem yet, so I'd appreciate any advice you can offer there.

samueljsb added a commit to samueljsb/silence-lint-error that referenced this issue Dec 11, 2023
This is necessary as a workaround to a bug in fixit
(Instagram/Fixit#405).
@samueljsb
Copy link
Contributor Author

samueljsb commented Dec 21, 2023

I have found another case. This looks superficially similar to the comprehensions case.

In the following file (t.py), we would expect the violation to be reported for the first function, but not for either of the other two.

def f(x):
    return (
        isinstance(x, int) or isinstance(x, float)
    )


def f(x):
    return (
        isinstance(x, int) or isinstance(x, float)  # lint-fixme: CollapseIsinstanceChecks
    )


def f(x):
    return (
        # lint-fixme: CollapseIsinstanceChecks
        isinstance(x, int) or isinstance(x, float)
    )

However, the violation is reported for all three:

$ fixit lint t.py
t.py@3:8 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
t.py@9:8 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
t.py@16:8 CollapseIsinstanceChecks: Multiple isinstance calls with the same target but different types can be collapsed into a single call with a tuple of types. (has autofix)
🛠️  1 file checked, 1 file with errors, 3 auto-fixes available 🛠️

@amyreese amyreese linked a pull request Apr 20, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant