Refactor check() #7288
Merged
Refactor check() #7288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanielNoord
added
Blocked 🚧
Pull Request Test Coverage Report for Build 2875868156
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
2 tasks
DanielNoord
force-pushed
the
lint-discovery-2
branch
from
de60d26
to
e79324d
Compare
Pierre-Sassoulas
requested changes
Aug 14, 2022
pylint/lint/pylinter.py
Outdated
Comment on lines
735
to
742
| except astroid.AstroidError as ex: | ||
| template_path = prepare_crash_report( | ||
| ex, fileitem.filepath, self.crash_file_path | ||
| ) | ||
| msg = get_fatal_error_message(fileitem.filepath, template_path) | ||
| self.add_message( | ||
| "astroid-error", args=(fileitem.filepath, msg), confidence=HIGH | ||
| ) |
There was a problem hiding this comment.
Not sure here, but did we remove the distinction between pylint error and astroid error ? It seems the "fatal" message can't be raised anymore.
There was a problem hiding this comment.
I think this still exists? It seems to be covered?
There was a problem hiding this comment.
I made a search for "fatal" and found nothing. I think we'd raise an 'astroid-error' now instead of 'fatal' ? Maybe we don't need the distinction and can simplify (the suggestion of new error was to keep that behaviour)
There was a problem hiding this comment.
get_fatal_error_message is just a helper message that returns a string that points to the template. I don't think we need to change anything here.
There was a problem hiding this comment.
I'm thinking of self.add_message("astroid-error", , previously there was a self.add_message("fatal", that is not raised anymore. I.e. The new code always raise an astroid-error, even when the error come from pylint or somewhere else like an OSError "no space in device", or something unexpected.
There was a problem hiding this comment.
Oh, my bad. I didn't read this correctly. You're right, this is a mistake. Fix incoming.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| assert not err | ||
| files = tmpdir.listdir() | ||
| assert len(files) == 1 | ||
| assert "pylint-crash-20" in str(files[0]) |
There was a problem hiding this comment.
Will python/pylint still be relevant in 2100 ? If so we're creating work for the next generation of maintainers 😄
There was a problem hiding this comment.
Copied from the test above 😄
DanielNoord
commented
Aug 15, 2022
| with open(files[0], encoding="utf8") as f: | ||
| content = f.read() | ||
| assert "Failed to import module spam." in content | ||
| assert any(m.symbol == "fatal" for m in linter.reporter.messages) |
There was a problem hiding this comment.
Previous test makes no sense as this test tries to lint the current file. Since the crash report includes the current file in the report whatever you put in an assertion will always end up in the crash report as well.
Since the test doesn't matter that much anyway I thought replacing was best.
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas Gentle ping as this is very prone to creating merge conflicts and I'd like to merge quickyl |
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on pandas:
The following messages are no longer emitted:
Effect on sentry:
The following messages are no longer emitted:
This comment was generated for commit 1147c08 |
Pierre-Sassoulas
approved these changes
Aug 17, 2022
No worries, just thought you might have missed this as you had reviewed more recent PRs. Thanks for the review 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Changes
Description
Refs #7263.
Blocked by #7286.