-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add related and details.rst to 'redundant-unittest-assert' style message's doc #6483
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 related and details.rst to 'redundant-unittest-assert' style message's doc #6483
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the intention of this PR I think. If we want people to know about a similar but different message we could probably add a link in the details.rst
of both of these messages.
|
||
def test_division(): | ||
a = 9 / 3 | ||
assert "No ZeroDivisionError were raised" # assert-on-string-literal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding an example for another message here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an example expecting this code to raise a [redundant-unittest-assert]
, but I think those two messages should be the same one. I think it can illustrate the two ways to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this example is about unittest
assertions, not normal assert
. I don't think we should merge those.
@@ -3,5 +3,9 @@ | |||
|
|||
class DummyTestCase(unittest.TestCase): | |||
def test_dummy(self): | |||
actual = "test_result" | |||
self.assertEqual(actual, "expected") | |||
# Nothing, as an assert of a string literal will always pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually kind of like the current example. I think the comment works nicely in details.rst
.
|
||
def test_division(): | ||
a = 9 / 3 | ||
assert a == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do # nothing
above we should also do so here? Note, I actually think we should keep the code instead of the comment.
6eb1909
to
e73767a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
details
of assert-on-tuple
should probably also be updated.
Co-authored-by: Daniël van Noord <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Daniël van Noord <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @DanielNoord this looks better indeed.
Type of Changes
Description
Follow up to a late review in #6475. I realized that two very close messages where on two different checkers it's minor and hard to fix (it could be just an old name for 'redundant-unittest-assert' to add to ' assert-on-string-literal' otherwise ).