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.

Sign up for GitHub

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

Jump to bottom

Check documentation examples for fatal errors #6475

Merged
merged 8 commits into from
Apr 30, 2022
Merged

Check documentation examples for fatal errors #6475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5b3c20b
Check documentation examples for fatal errors
jacobtylerwalls Apr 29, 2022
5cc55bc
Update in both places
jacobtylerwalls Apr 29, 2022
c488eba
Merge branch 'main' into doc-fatal-errors
jacobtylerwalls Apr 29, 2022
6a41897
Update assertion examples to avoid syntax errors
jacobtylerwalls Apr 29, 2022
5b8eed5
restore mypy disables. version diff?
jacobtylerwalls Apr 29, 2022
158ceb3
oops
jacobtylerwalls Apr 29, 2022
0cd085a
Add details.rst
jacobtylerwalls Apr 30, 2022
13f1ac8
another details.rst
jacobtylerwalls Apr 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/data/messages/a/assert-on-string-literal/details.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Directly asserting a string literal will always pass.
4 changes: 3 additions & 1 deletion doc/data/messages/a/assert-on-string-literal/good.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
def test():
# Nothing, as an assert of a string literal will always pass
result = "result"
expected = "expected"
assert result == expected
1 change: 1 addition & 0 deletions doc/data/messages/r/redundant-unittest-assert/details.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Directly asserting a string literal will always pass.
3 changes: 2 additions & 1 deletion doc/data/messages/r/redundant-unittest-assert/good.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@

class DummyTestCase(unittest.TestCase):
def test_dummy(self):
# Nothing, as an assert of a string literal will always pass
actual = "test_result"
self.assertEqual(actual, "expected")
Comment on lines +6 to +7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
actual = "test_result"
self.assertEqual(actual, "expected")
# Nothing, as an assert of a string literal will always pass
pass

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this is just taste and could go either way, but I was figuring the most likely mistake a beginner makes is not "oh I don't need to assert in the first place" but "oops I was trying to assert a comparison but I forgot to put the right hand side of it". So the solution is to put in the right hand side.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense, Daniel also think this example was better so we kept it in the follow-up MR :)

2 changes: 1 addition & 1 deletion doc/test_messages_documentation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def __init__(self, test_file: Tuple[str, Path]) -> None:
args_list=[
str(test_file[1]),
"--disable=all",
f"--enable={test_file[0]}",
f"--enable={test_file[0]},astroid-error,fatal,syntax-error",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"--enable={test_file[0]},astroid-error,fatal,syntax-error",
f"--enable={test_file[0]},F,E",

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I don't think we want to enable all errors, though. Just the fatal ones from pylint/astroid breaking, not antipatterns in the code under test.

Copy link
Member

Choose a reason for hiding this comment

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

What would be an example of error we want to keep when we're not specifically documenting it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I thought I remembered the original discussion was "don't try to pass every pylint check" but you have a point that maybe errors are worth avoiding. Things like undefined-variable can be tedious to avoid if you have initialize all your thing and my_list etc. Let me see what fails if we add this.

Copy link
Member Author

Choose a reason for hiding this comment

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

some examples (there are more):

doc/data/messages/w/wrong-import-order/good.py:6:0: E0611: No name 'utils' in module '' (no-name-in-module)
doc/data/messages/y/yield-inside-async-function/good.py:6:0: E0102: function already defined line 1 (function-redefined)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah wrong-import-order is possible to do if we remove the "our own lib" part but then it would be incomplete. For the other one we might need to find a way to propose multiple solution to the same bad.py but this is a non trivial refactor to do. I was thinking generally that being forced to create example without error would make the examples better but let's revisit that later when the cost to do it is lower.

Copy link
Member Author

Choose a reason for hiding this comment

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

The boundary is probably subjective. I asked a contributor to fix an easy TypeError from not passing an argument to a function in #6472

Copy link
Member

Choose a reason for hiding this comment

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

We could also explicitly disable the errors in the example ? (# pylint: disable=no-name-in-module for example) Would prevent us from not seeing that there's an issue in an example.

],
reporter=_test_reporter,
config_file=config_file,
Expand Down
1 change: 1 addition & 0 deletions pylint/testutils/lint_module_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ def __init__(
# Always enable fatal errors
messages_to_enable.add("astroid-error")
messages_to_enable.add("fatal")
messages_to_enable.add("syntax-error")
args.extend(["--disable=all", f"--enable={','.join(messages_to_enable)}"])
_config_initialization(
self._linter, args_list=args, config_file=rc_file, reporter=_test_reporter
Expand Down
1 change: 1 addition & 0 deletions tests/testutils/test_functional_testutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def test_minimal_messages_config_enabled(pytest_config) -> None:
# Always enable fatal errors: important not to have false negatives
"astroid-error",
"fatal",
"syntax-error",
)
)
assert not mod_test._linter.is_message_enabled("unused-import")
Expand Down