Skip to content

[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

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jun 22, 2022

Type of Changes

Type
🔨 Refactoring

Description

Refs #6984, blocked by #7077

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jun 22, 2022
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft June 22, 2022 08:25
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the use-real-message-class-in-primer branch from e252f7d to 6e42f45 Compare June 22, 2022 08:27
@coveralls
Copy link

coveralls commented Jun 22, 2022

Pull Request Test Coverage Report for Build 2581919387

  • 5 of 12 (41.67%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 95.342%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/testutils/_primer/primer_run_command.py 2 9 22.22%
Files with Coverage Reduction New Missed Lines %
pylint/testutils/_primer/primer_run_command.py 1 39.02%
Totals Coverage Status
Change from base Build 2581704758: 0.01%
Covered Lines: 16682
Relevant Lines: 17497

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the use-real-message-class-in-primer branch from 6e42f45 to fce0e86 Compare June 24, 2022 08:37
@Pierre-Sassoulas
Copy link
Member Author

@DanielNoord before I really investigate this, do you have any idea why the installation fail for this change ? (https://github.com/PyCQA/pylint/runs/7038428338?check_suite_focus=true) It seems strange that it fail at the install stage, but it's fairly consistent across runs.

@DanielNoord
Copy link
Collaborator

DanielNoord commented Jun 24, 2022

I don't think it fails at the install stage. Note that that is also the run stage. Have you tried this locally?

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the use-real-message-class-in-primer branch from fce0e86 to bc9cb35 Compare June 27, 2022 15:02
@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Jun 27, 2022
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review June 27, 2022 15:26
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit bc9cb35

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the use-real-message-class-in-primer branch from bc9cb35 to 4a3d2bf Compare June 29, 2022 08:57
@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Jun 29, 2022
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 4a3d2bf

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":
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

json.dump(packages, f)
json.dump(
{
p: [JSONReporter.serialize(m) for m in msgs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this serialisation even necessary? Aren't TypedDict dumpable and loadable by default?

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jun 29, 2022

Choose a reason for hiding this comment

The 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 pylint.message.Message instance because when we load the json we deserialize.

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 we can't we remove both of those? And just store the TypedDict in the json?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use the TypedDict we introduced in #7077 yes, but Message is used everywhere in the code base.

Copy link
Member Author

Choose a reason for hiding this comment

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

And there an immutable location in it that I'd like to use later on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I missed the JSONReporter serialises all of its messages before printing anyway. That makes this necessary.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 66e1223 into pylint-dev:main Jun 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the use-real-message-class-in-primer branch June 29, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants