Skip to content

[primer] Create a class for easier comparison of pylint results #6984

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 41 additions & 0 deletions pylint/testutils/_primer/comparator.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt

from __future__ import annotations

import json
from collections.abc import Generator
from pathlib import Path

from pylint.testutils._primer.primer_command import Messages, PackageMessages


class Comparator:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rename this to PylintComparator? At some point I want to make an AstroidComparator 😄

If you're up for it perhaps we could even create an abstract class?

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jun 22, 2022

Choose a reason for hiding this comment

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

What do you mean by astroid comparator ? Comparing a json dump of the generated ast ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, using the primer on astroid/main to catch crashes there before we merge them. That would require a different Comparator I think.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jun 22, 2022

Choose a reason for hiding this comment

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

So it's the basic primer for pylint but for astroid ? It does not need a comparator right ? As if a crash exists it's always problem, and there's no crash on main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but I thinking of maybe running the pylint test suite on some version on astroid main or something like that. The overall goal is to remove the need for pylint-tested label and have all PRs get that automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure I understood that :) It make sense to test astroid more in the astroid CI, because pylint's CI is already crowded so I think it's a really good idea too.

def __init__(self, main_json: Path, pr_json: Path):
main_messages = self._load_json(main_json)
self.pr_messages = self._load_json(pr_json)
self.missing_messages: PackageMessages = {}
for package, messages in main_messages.items():
self.missing_messages[package] = []
for message in messages:
try:
self.pr_messages[package].remove(message)
except ValueError:
self.missing_messages[package].append(message)

def __iter__(
self,
) -> Generator[tuple[str, Messages, Messages], None, None]:
for package, missing_messages in self.missing_messages.items():
new_messages = self.pr_messages[package]
if not missing_messages and not new_messages:
print(f"PRIMER: No changes in {package}.")
continue
yield package, missing_messages, new_messages

@staticmethod
def _load_json(file_path: Path | str) -> PackageMessages:
with open(file_path, encoding="utf-8") as f:
result: PackageMessages = json.load(f)
return result
52 changes: 9 additions & 43 deletions pylint/testutils/_primer/primer_compare_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,65 +3,31 @@
# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt
from __future__ import annotations

import json
from pathlib import Path, PurePosixPath
from pathlib import PurePosixPath

from pylint.reporters.json_reporter import OldJsonExport
from pylint.testutils._primer.primer_command import (
PackageData,
PackageMessages,
PrimerCommand,
)
from pylint.testutils._primer.comparator import Comparator
from pylint.testutils._primer.primer_command import Messages, PrimerCommand

MAX_GITHUB_COMMENT_LENGTH = 65536


class CompareCommand(PrimerCommand):
def run(self) -> None:
main_data = self._load_json(self.config.base_file)
pr_data = self._load_json(self.config.new_file)
missing_messages_data, new_messages_data = self._cross_reference(
main_data, pr_data
)
comment = self._create_comment(missing_messages_data, new_messages_data)
comparator = Comparator(self.config.base_file, self.config.new_file)
comment = self._create_comment(comparator)
with open(self.primer_directory / "comment.txt", "w", encoding="utf-8") as f:
f.write(comment)

@staticmethod
def _cross_reference(
main_data: PackageMessages, pr_data: PackageMessages
) -> tuple[PackageMessages, PackageMessages]:
missing_messages_data: PackageMessages = {}
for package, data in main_data.items():
package_missing_messages: list[OldJsonExport] = []
for message in data["messages"]:
try:
pr_data[package]["messages"].remove(message)
except ValueError:
package_missing_messages.append(message)
missing_messages_data[package] = PackageData(
commit=pr_data[package]["commit"], messages=package_missing_messages
)
return missing_messages_data, pr_data

@staticmethod
def _load_json(file_path: Path | str) -> PackageMessages:
with open(file_path, encoding="utf-8") as f:
result: PackageMessages = json.load(f)
return result

def _create_comment(
self, all_missing_messages: PackageMessages, all_new_messages: PackageMessages
) -> str:
def _create_comment(self, comparator: Comparator) -> str:
comment = ""
for package, missing_messages in all_missing_messages.items():
for package, missing_messages, new_messages in comparator:
if len(comment) >= MAX_GITHUB_COMMENT_LENGTH:
break
new_messages = all_new_messages[package]
if not missing_messages["messages"] and not new_messages["messages"]:
continue
comment += self._create_comment_for_package(
package, new_messages, missing_messages
package, missing_messages, new_messages
)
comment = (
f"🤖 **Effect of this PR on checked open source code:** 🤖\n\n{comment}"
Expand All @@ -74,7 +40,7 @@ def _create_comment(
return self._truncate_comment(comment)

def _create_comment_for_package(
self, package: str, new_messages: PackageData, missing_messages: PackageData
self, package: str, missing_messages: Messages, new_messages: Messages
) -> str:
comment = f"\n\n**Effect on [{package}]({self.packages[package].url}):**\n"
# Create comment for new messages
Expand Down
2 changes: 1 addition & 1 deletion tests/testutils/_primer/test_primer.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
PACKAGES_TO_PRIME_PATH = TEST_DIR_ROOT / "primer/packages_to_prime.json"
FIXTURES_PATH = HERE / "fixtures"

PRIMER_CURRENT_INTERPRETER = (3, 10)
PRIMER_CURRENT_INTERPRETER = (3, 8)

DEFAULT_ARGS = ["python tests/primer/__main__.py", "compare", "--commit=v2.14.2"]

Expand Down