flake8-bugbear icon indicating copy to clipboard operation
flake8-bugbear copied to clipboard

Possible B020 false positive with instance attribute

Open MrkGrgsn opened this issue 3 years ago • 3 comments

This code:

class MyModel:
    value: int

class A:
    model_instance = MyModel()
    test_suite = [1, 2, 3]

    def method(self):
        for self.model_instance.value in self.test_suite:
            print(self.model_instance.value)

results in a B020, but it looks like a false positive to me since self.test_suite is not one of the iterable values. Admittedly it is a slightly unusual and ambiguous case since self is being modified and the plugin can't know what changes setting self.model_instance is making to the object and the value of self.test_suite.

The actual use case is a test where a Django model object is created once in the test set up and, in each iteration, the loop modifies model attributes, passes the model to a function, and tests the result.

MrkGrgsn avatar Mar 30 '22 04:03 MrkGrgsn

I agree this seems a false positive, but it's very creative class instance iteration here. It's also a class variable, so all instances would modify the same value. Dangerous stuff.

To potentially fix this noise will need some thought. I'm not even sure if we can. Let's see what others think.

cooperlees avatar Mar 30 '22 22:03 cooperlees

This one's not my code! It's definitely creative and I had to look twice because I'd not seen such a pattern before.

Note that the B020 is raised if it's an instance variable also, in fact it is in my case but I was trying to make the example simpler.

The real case is like this.

class MyTestCase(TestCase):
    test_suite = [1, 2, 3]

    def setUp(self):
        self.model_instance = MyModel.objects.get(...)

    def test_method(self):
        for self.model_instance.value in self.test_suite:
            print(self.model_instance.value)

I don't see a particularly compelling reason for it to be written this way. An alternative, as an example, would be for the test_method to instantiate a new MyModel each iteration without writing to the DB. But even if it should be written a different way it still concerns me that B020 is an inaccurate description of the style issue.

MrkGrgsn avatar Mar 30 '22 23:03 MrkGrgsn

Introducing another code for this would be helpful (or just ignoring) IMHO.

spaceone avatar Jan 26 '23 19:01 spaceone