-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[primer] Refactor the primer to use 'pylint.message.Message' #7000
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from itertools import chain | ||
|
||
from pylint.lint import Run | ||
from pylint.message import Message | ||
from pylint.reporters import JSONReporter | ||
from pylint.testutils._primer.package_to_lint import PackageToLint | ||
from pylint.testutils._primer.primer_command import PackageMessages, PrimerCommand | ||
|
@@ -24,20 +25,18 @@ def run(self) -> None: | |
packages: PackageMessages = {} | ||
|
||
for package, data in self.packages.items(): | ||
output = self._lint_package(data) | ||
packages[package] = output | ||
packages[package] = self._lint_package(data) | ||
print(f"Successfully primed {package}.") | ||
|
||
astroid_errors = [] | ||
other_fatal_msgs = [] | ||
for msg in chain.from_iterable(packages.values()): | ||
if msg["type"] == "fatal": | ||
# Remove the crash template location if we're running on GitHub. | ||
# We were falsely getting "new" errors when the timestamp changed. | ||
assert isinstance(msg["message"], str) | ||
if GITHUB_CRASH_TEMPLATE_LOCATION in msg["message"]: | ||
msg["message"] = msg["message"].rsplit(CRASH_TEMPLATE_INTRO)[0] | ||
if msg["symbol"] == "astroid-error": | ||
if msg.category == "fatal": | ||
if GITHUB_CRASH_TEMPLATE_LOCATION in msg.msg: | ||
# Remove the crash template location if we're running on GitHub. | ||
# We were falsely getting "new" errors when the timestamp changed. | ||
msg.msg = msg.msg.rsplit(CRASH_TEMPLATE_INTRO)[0] | ||
if msg.symbol == "astroid-error": | ||
astroid_errors.append(msg) | ||
else: | ||
other_fatal_msgs.append(msg) | ||
|
@@ -48,7 +47,13 @@ def run(self) -> None: | |
"w", | ||
encoding="utf-8", | ||
) as f: | ||
json.dump(packages, f) | ||
json.dump( | ||
{ | ||
p: [JSONReporter.serialize(m) for m in msgs] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this serialisation even necessary? Aren't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a typed dict at this point, it's an actual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but we can't we remove both of those? And just store the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use the TypedDict we introduced in #7077 yes, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And there an immutable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I missed the |
||
for p, msgs in packages.items() | ||
}, | ||
f, | ||
) | ||
|
||
# Fail loudly (and fail CI pipelines) if any fatal errors are found, | ||
# unless they are astroid-errors, in which case just warn. | ||
|
@@ -59,7 +64,7 @@ def run(self) -> None: | |
warnings.warn(f"Fatal errors traced to astroid: {astroid_errors}") | ||
assert not other_fatal_msgs, other_fatal_msgs | ||
|
||
def _lint_package(self, data: PackageToLint) -> list[dict[str, str | int]]: | ||
def _lint_package(self, data: PackageToLint) -> list[Message]: | ||
# We want to test all the code we can | ||
enables = ["--enable-all-extensions", "--enable=all"] | ||
# Duplicate code takes too long and is relatively safe | ||
|
@@ -69,4 +74,4 @@ def _lint_package(self, data: PackageToLint) -> list[dict[str, str | int]]: | |
output = StringIO() | ||
reporter = JSONReporter(output) | ||
Run(arguments, reporter=reporter, exit=False) | ||
return json.loads(output.getvalue()) | ||
return [JSONReporter.deserialize(m) for m in json.loads(output.getvalue())] |
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.
Is category the same as type?
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.
Yes, as per https://github.com/PyCQA/pylint/blob/dee2f1449139aa013906cec685c05a8c1d5e6360/pylint/reporters/json_reporter.py#L85