[PWGLF] update of kstarInOO.cxx

[JimunLee]

Dear experts,

The jet Analysis part has been added.

Thank you,
Jimun

[github-actions]

O2 linter results: ❌ 112 errors, ⚠️ 54 warnings, 🔕 0 disabled

[vkucera]

Please use a meaningful PR title and fix the errors and warnings.

[vkucera]

@smaff92 Please don't merge PRs which introduce errors or have vague titles.

[smaff92]

Hi @vkucera,

Sorry, maybe I missed something, but the latest push cleared all checks except for O2 linter (which is not mandatory), which is fine, no?

With respect to the title, I sort-of agree that the title is a bit vague, but simultaneously, looking through the actual changes to the code, it is a large overhaul over many different areas of the code, so it is difficult to pinpoint the title to a single change. What name would you have suggested for the title, for future reference?

Best,
Adrian

[vkucera]

Hi @smaff92,

the latest push cleared all checks except for O2 linter (which is not mandatory), which is fine, no?

No, it is not fine to ignore errors or warnings. Their very purpose is to report things that are not fine. Errors are top-priority issues that should be fixed immediately before merging, warnings are lower-priority issues that can be fixed later. The fact that it does not block the merging does not mean it can be ignored.

What name would you have suggested for the title, for future reference?

It is up to @JimunLee to summarise what she did. We have guidelines and links to references about writing a good commit message. As a general rule of thumb, if it is not possible to summarise the changes in a brief message, it usually means that the changes are too big for a single PR.
The PR description says: "The jet Analysis part has been added." so a good PR title could be: "Add jet analysis in kstarInOO". But from what I can see, that would not be true because the jet analysis parts are already there.

[smaff92]

Dear @vkucera,

I am sorry; not trying to pick a fight here, but I strongly disagree with some of your points.
Firstly, in our GIT system, we have two different categories of tests:

1.) Tests that are REQUIRED
2.) Tests that are non required.

Operating with the idea that the non-required checks are still pseudo-required is confusing to analyzers and code-owners alike. In this context, what is even the point of having the REQUIRED status?

Secondly,

No, it is not fine to ignore errors or warnings. Their very purpose is to report things that are not fine.

This logic is entirely cyclical. You are appealing to these being errors that should be fixed, while you/the O2linter team are the ones defining them as errors. If you think these warnings/error are mission critical, I feel you should either have to:

1.) Include the mission critical checks into as task that is already REQUIRED
2.) Mark O2linter as REQUIRED.

Now, every code-owner has their own perspective, but for people that come to me asking to approve their PR's, I will consider:

A.) Are the REQUIRED checks passed?
B.) Does the code compile?
C.) In the case of non-required checks not passing, I would suggest the user to implement them in their next PR, because they are exactly that; suggestions.

If you can point me to a statement within ALICE management/Physics Board that contradicts my approach, I will adjust accordingly.

Lastly, I am a bit confused here:

It is up to @JimunLee to summarise what she did.... means that the changes are too big for a single PR.

I don't understand what the prescription here is? As you have yourself identified, this PR was broad enough where it was difficult to have a precise title? I mean I agree with you; this single PR is probably too inclusive. But then the prescription should not be "change title", but rather "please make smaller and more precise titles in the future", no?

Best Regards,
Adrian.N

[vkucera]

I will try to clarify.

we have two different categories of tests:

1.) Tests that are REQUIRED
2.) Tests that are non required.

... what is even the point of having the REQUIRED status?

You can see the "required" and "non required" checks as "critical" and "non critical" for merging the PR, respectively, where the "non critical" category includes multiple tools with different ways of reporting severity levels. Since GitHub only supports pass and fail states of GitHub actions, we try to configure the tools to fail on the more important issues and report the rest as warnings. (That does not make the warnings unimportant.)

No, it is not fine to ignore errors or warnings. Their very purpose is to report things that are not fine.

This logic is entirely cyclical. You are appealing to these being errors that should be fixed, while you/the O2linter team are the ones defining them as errors.

What is cyclical about that?

If you think these warnings/error are mission critical, I feel you should either have to:

1.) Include the mission critical checks into as task that is already REQUIRED
2.) Mark O2linter as REQUIRED.

This is a bad idea for several reasons:

• 1. They are not mission-critical.
• 2. Marking it as required now would mean that suddenly the next PR author has to fix all issues ignored by all other people who had touched the files before, which, I hope we agree, would not be fair.
• 3. We use third-party tools which are not fully under our control so we want to avoid blocking all PRs in case of sudden problems with the checks.

Now, every code-owner has their own perspective, but for people that come to me asking to approve their PR's, I will consider:

A.) Are the REQUIRED checks passed? B.) Does the code compile? C.) In the case of non-required checks not passing, I would suggest the user to implement them in their next PR, because they are exactly that; suggestions.

• 1. I don't see any such suggestion from you in this PR.
• 2. If by "suggestion" you mean something that is fine to ignore, you misunderstand the nature of the error/warning messages. Please see my clarification above.
• 3. You are supposed to review the code changes. The CI checks are there to help you with that.

If you can point me to a statement within ALICE management/Physics Board that contradicts my approach, I will adjust accordingly.

The decisions have been made at the WP 14 meetings in agreement with the Analysis Coordinator. Please see the references in the O2 linter log and in the documentation.

But then the prescription should not be "change title", but rather "please make smaller and more precise titles in the future", no?

I have tried to provide clear guidelines and references on this topic on the page with contribution guidelines. I will appreciate suggestions for improvements.

[smaff92]

Dear @vkucera,

Let's step back a second. I am not stating that the O2 linter is not justified, or attacking any of the WP14 decisions.

This started here:

Please don't merge PRs which introduce errors....

I take this as an impugnment on my decision to merge this PR. Since the only errors which remained were from the O2 linter, I take the it to mean:

Please don't merge PRs which introduce O2 linter errors....

So now, the question is, can you quote me to any directive that states that I, as a code-owner, am not authorized to merge PR's unless they pass all non-required checks? Because from your own statements, these checks are

A.) Not mission-critical
B.) Will not interfere with the build itself/other people's builds
C.) In the decision in WP14, it was explicitly stated that adhering to the O2 linter would not be mandatory.

From this, I view it that the errors in the O2 linter should be treated as recommendations, not something I would block a PR for. I am not saying people should ignore these; I think a lot of them are good and I would suggest some of them. I am merely stating that I will not enforce that people implement these suggestions for a PR to go through. Unless you can provide me a directive that says otherwise, I will continue operating like this.

Marking it as required now would mean that suddenly the next PR author has to fix all issues ignored by all other people who had touched the files before, which, I hope we agree, would not be fair.

This is literally what you're doing; by telling me that I should not accept the PR unless the O2 linter errors are cleared is functionally the same as having these as REQUIRED. The difference is that with having it listed as REQUIRED, there is a clear understanding for both code-owner and user what is expected. Currently it is not -- You are suggesting that I am expected to enforce these non-required things as if they required to accept the PR.

I don't see any such suggestion from you in this PR.

It does not have to be in GIT or the PR; I usually communicate my suggestions over mattermost when people ping me for a PR request. Whether people follow any suggestions that are not mission critical (my own, or the non-mandatory checks), I leave up to the analyzer to decide.

Best Regards,
Adrian.N

[vkucera]

Sorry but I don't understand your reasoning. We have checks approved at the collaboration level which tell you: "This is bad. Fix it." and "This could be improved in this way." and your position seems to be: "I am not going to request these fixes and improvements unless I am ordered to do so." which is equivalent to ignoring them. Why do you need an explicit order to enforce a decision that has already been taken?

Marking it as required now would mean that suddenly the next PR author has to fix all issues ignored by all other people who had touched the files before, which, I hope we agree, would not be fair.

This is literally what you're doing; by telling me that I should not accept the PR unless the O2 linter errors are cleared is functionally the same as having these as REQUIRED.

Please don't distort my words. I did not say that. I said "Please don't merge PRs which introduce errors", i.e. "add new errors".