-
Notifications
You must be signed in to change notification settings - Fork 158
Fix wrong warnings about bad flags assigned to good qualities #2583
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
|
Hello @singiamtel , @ktf , it seems that our builds are all broken: o2 and o2-dataflow-slc9 do not report any status, while macOS-arm fails: could you please have a look? |
|
Hi @knopers8, we're investigating |
|
Thanks! |
Due to the order of processing in WorstOfAllAggregator, we could get cases when a bad Flag (coming from Bad quality) would be assigned to Good quality, while the quality itself would be replaced just after. This commit reverses the order, which will silence the wrong warning. The same code was also reused TrendCheck, thus I am fixing it also there.
aa8b935 to
1d85d6b
Compare
|
@knopers8 the issue should be fixed now. Thanks for the report |
| } | ||
| for (const auto& flag : quality.getFlags()) { | ||
| result.addFlag(flag.first, flag.second); | ||
| } |
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 am sorry but it isn't obvious for me how just reversing order of these two operations in one loop would fix the issue. you are creating Quality object, setting it's quality to the one you find as worse (finding the worst) ... aka setting it's level and name... and than adding all flags from all of the qualities encountered in check. So unless I am missing something I don't understand how you achieve with different situation than before.
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.
nevermind, I understand, there are internal checks in addFlag ... but just reading the PR isn't obvious why this should fix anything. I would expect that addFlag is just adding flag without any internal checks.
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 think it's quite normal that functions validate their inputs, but indeed I could have mentioned where the warning was coming from.
Due to the order of processing in WorstOfAllAggregator, we could get cases when a bad Flag (coming from Bad quality) would be assigned to Good quality, while the quality itself would be replaced just after. This commit reverses the order, which will silence the wrong warning.