[fixed] call method for test #179
Conversation
| @@ -22,7 +22,7 @@ | |||
| setattr(foo, bar, None) | |||
| setattr(foo, "bar{foo}".format(foo="a"), None) | |||
| setattr(foo, "123abc", None) | |||
| getattr(foo, "except", None) | |||
| setattr(foo, "except", None) | |||
There was a problem hiding this comment.
Can we get a little explanation here - Why didn't we have to update any bad lines if this was wrong? What's it achieving - Just making sure we test that this passes as valid right?
There was a problem hiding this comment.
I consider this code to be a sample as well as a test.
And I learned B009 and B010 from this code.
At that time, I had a comment on line 21 "Valid setattr usage" which I thought was a typo and fixed it.
Sorry for the empty overview. I will describe this content.
There was a problem hiding this comment.
| setattr(foo, "except", None) | |
| getattr(foo, "except", None) | |
| setattr(foo, "except", None) |
I don't get what's incorrect here and what extra this line brings to the unittest. We purposely put a mix of code that we want to make sure it does not fire on so the getattr is on purpose (to show we don't fire as it has a default) due to the file being for both b009 + b010. So I feel we should keep both at least.
I also feel the setattr you're adding is basically the same as line 24 ... What makes it different?
Unless you can explain to me how this makes things better I am going to close this PR. Thanks for your effort here, but I feel a little more description on what this is doing and how it improves things is needed.
(I will accept I'm missing something too)
There was a problem hiding this comment.
I'm going to lay out what I'm trying to say.
- On line 7, there is a comment "Valid getattr usage".
- On line 21, there is a comment "Valid setattr usage".
- In the "Valid setattr usage" section, "getattr" is mixed in.
- If this line of code is necessary, it should be moved to the section on line 7.
Sorry to waste your time on a small PR campaign.
It's not a big deal, so you can close it if you're not interested.
Thanks for the comment.
|
Agree with @yamap55, the test was supposed to show |
Fixed a call to "getattr" in "Valid setattr usage".