MartinThoma / flake8-simplify

❄ A flake8 plugin that helps you to simplify code
MIT License
186 stars 19 forks source link

[Adjust Rule] SIM119 - Use a dataclass rule #63

Closed Kub-AT closed 2 years ago

Kub-AT commented 3 years ago

Desired change

Explanation

Examples below are suggested as dataclass. I think they shouldnt

Example

class RegexAssert:
    def __init__(self, pattern, flags=0):
        self._regex = re.compile(pattern, flags)

    def __eq__(self, actual):
        return bool(self._regex.match(actual))

    def __repr__(self):
        return self._regex.pattern
class OfType:
    """
    >>> 3 == OfType(int, str, bool)
    True
    >>> 'txt' == OfType(int)
    False
    """

    def __init__(self, *types):
        self.type = types

    def __eq__(self, other):
        return isinstance(other, self.type)
dsegan commented 2 years ago

Even simpler examples that SIM119 is raised for:

class Foo:
    def __init__(self, a):
        self.b = a + 4

Or

class Bar:
    def __init__(self, a):
        self.a = a + 4

    async def foo(self):
        pass

Basically, if that foo() is turned into a non-async method, SIM119 is not raised.

So it seems there are at least 3 cases where SIM119 should not be suggested:

  1. __init__ actually processes passed-in value before setting it on the class
  2. dunder methods should stop considering a class from being a good candidate for a dataclass
  3. async methods should just as well
dsegan commented 2 years ago

Linked PR will not make the OfType example not trigger the warning.

MartinThoma commented 2 years ago

Here is a test which shows the issue:

def test_sim119_false_positive():
    results = _results('''class OfType:
    """
    >>> 3 == OfType(int, str, bool)
    True
    >>> 'txt' == OfType(int)
    False
    """

    def __init__(self, *types):
        self.types = types

    def __eq__(self, other):
        return isinstance(other, self.types)''')
    for el in results:
        assert "SIM119" not in el