-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Pull Request Test Coverage Report for Build 3260364257
π - Coveralls |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
doc/whatsnew/fragments/7214.bugfix
Outdated
@@ -0,0 +1,3 @@ | |||
Message send to reporter are now copied so a reporter cannot modify the message sent to other reporters. | |||
|
|||
Refs #7214 |
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.
should this be Closes
?
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.
I wasn't sure of semantics of this, but yes ! Let me fix that.
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.
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.
I guess copy works well with dataclass instances, Message contains only litterals. |
Co-authored-by: Pierre Sassoulas <[email protected]>
This comment has been minimized.
This comment has been minimized.
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit ece4392 |
We could also change the |
@
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 ? |
Yeah you're right. If anybody complains about this and has a usecase we can always update to allow it again. |
Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>
Type of Changes
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