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

Add B015: pointless statement. #130

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

kalekseev
Copy link
Contributor

For me the main reason to have such check is pytest style asserts, they are quite easy to miss in the middle of the test function

def test_number of objects():
    ...setup code ...
    MyModel.objects.count() == 2   # should be `assert MyModel.objects.count() == 2`
    ...more asserts...

so tests are green and developer thinks he's asserted important results in the test.

Pylint has similar rule pointless-statement although it's more advanced but it's a pain to run pylint on tests and not all developers can tolerate false positives from it, so I think it would be a good addition to the flake8-bugbear.

Right now that rule checks only comparisons so maybe error should be: "pointless comparison" or we may try to make the rule more complete by including other cases, eg. uncalled functions:

service = Service()
service.run  # should be service.run()
...

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi - Thanks! I like this check.

Can you add a description to the README like other checks please?

Other than that it looks good. This also reminds me I need to release a new version. So I'll do that once we merge this :)

@cooperlees
Copy link
Collaborator

Also, lets make the name specific (i.e. pointless comparisons) since all it does is comparisons ...

@kalekseev kalekseev force-pushed the feature/pointless-statement branch from ff2e784 to ef297c7 Compare July 10, 2020 14:38
@kalekseev
Copy link
Contributor Author

Done, though error looks a bit terse relative to others, not sure if we should add more details or is it too obvious to explain?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just maybe be a little more verbose and I'll merge.

README.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only considering my verbose message for the README. But can't hurt to be verbose there too.

Just need to make the linter happy on it's own code :P

bugbear.py Outdated Show resolved Hide resolved
@kalekseev kalekseev force-pushed the feature/pointless-statement branch from 0782e4b to f1472f3 Compare July 10, 2020 20:53
@cooperlees cooperlees merged commit 6498b93 into PyCQA:master Jul 10, 2020
@WhyNotHugo
Copy link

With the latest release, my CI system has picked up the latest version, and raised B015 for a few of tests.

Just wanna say thanks: this change helped me find 8 missing asserts in unit tests that could've let bugs go unnoticed!

@cooperlees
Copy link
Collaborator

Sir, Thankyou very much for sharing this. You often get complaints and not thanks. So it’s very nice to hear that everyone’s work here is helping people! Glad you have 8 more useful tests now!

@pquentin
Copy link

Yes, we fixed a number of tests thanks to this warning. Thanks a lot! 💯

@Zac-HD
Copy link
Member

Zac-HD commented Dec 1, 2020

Just dropping by to say that this also caught a single missing assert in Hypothesis' tests! Thanks to @kalekseev for this new check and @cooperlees for what I know is usually a thankless task maintaining things 🥰

@kalekseev
Copy link
Contributor Author

Thank you guys for such a great feedback, I appreciate that!

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 this pull request may close these issues.

5 participants