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.

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

fix: reporters receive copy of message #7620

Merged
merged 6 commits into from
Oct 31, 2022
Merged

fix: reporters receive copy of message #7620

merged 6 commits into from
Oct 31, 2022

Conversation

Smixi
Copy link
Contributor

@Smixi Smixi commented Oct 13, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

MultiReporter was sending message by reference, which then could be changed by reporter. The colorized reporter was one of them, leading to changing any other reporters. This was broken since 2.14.0 and affected mostly people using colorized with a custom reporter.

Closes #7214

@coveralls
Copy link

coveralls commented Oct 13, 2022

Pull Request Test Coverage Report for Build 3260364257

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 26 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.0008%) to 95.354%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/refactoring/refactoring_checker.py 6 98.2%
pylint/checkers/utils.py 20 95.23%
Totals Coverage Status
Change from base Build 3245605436: 0.0008%
Covered Lines: 17138
Relevant Lines: 17973

πŸ’› - Coveralls

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added Bug πŸͺ² Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Oct 14, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.5 milestone Oct 14, 2022
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Oct 14, 2022
@@ -0,0 +1,3 @@
Message send to reporter are now copied so a reporter cannot modify the message sent to other reporters.

Refs #7214
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be Closes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure of semantics of this, but yes ! Let me fix that.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for the investigation. I wonder what are the performance implication and also if the side effect of being able to modify other reporter's message wasn't seen as a feature by some users. We'd need to do it as a breaking change if that's the case.

pylint/reporters/multi_reporter.py Outdated Show resolved Hide resolved
@Smixi
Copy link
Contributor Author

Smixi commented Oct 14, 2022

I guess copy works well with dataclass instances, Message contains only litterals.
I don't know if anyone is using multiple formatter to change data, but I think each formatter should handle themselve the message they receive. For instance, colorized was breaking any logic a reporter would have for a Message, like handling a path, category, module ...

@github-actions

This comment has been minimized.

@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 ece4392

@DanielNoord
Copy link
Collaborator

We could also change the handle_message in the colour reporter to create a copy. That way reporters that depend on being able to modify for others aren't broken?

@Smixi
Copy link
Contributor Author

Smixi commented Oct 17, 2022

@

We could also change the handle_message in the colour reporter to create a copy. That way reporters that depend on being able to modify for others aren't broken?

Do you have an usecase example ? Maybe that would be another option ? One issue is that we would need solve is to specify ordering. From 2.13.9 to 2.14.0, coloriezd/text reporter is now always the first reporter. If two reporters are necessary to achieve a specific output, maybe it should be only one reporter with options ?

@DanielNoord
Copy link
Collaborator

Yeah you're right. If anybody complains about this and has a usecase we can always update to allow it again.

@cdce8p cdce8p modified the milestones: 2.15.5, 2.15.6 Oct 21, 2022
@Pierre-Sassoulas Pierre-Sassoulas merged commit 35ce253 into pylint-dev:main Oct 31, 2022
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Oct 31, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Nov 16, 2022
Pierre-Sassoulas added a commit that referenced this pull request Nov 16, 2022
Pierre-Sassoulas added a commit that referenced this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colorized output should not modify msg objects
6 participants