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.

Sign up for GitHub

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

doc: contribute: Extend Reviewer Expectations with additional rules #83176

Merged
merged 1 commit into from
Jan 17, 2025
+19 −2
Merged

doc: contribute: Extend Reviewer Expectations with additional rules #83176

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions doc/contribute/contributor_expectations.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,27 @@ Reviewer Expectations
they address all non-blocking comments. PR authors should acknowledge every
review comment in some way, even if it's just with an emoticon.

- Reviewers shall be *clear* and *concise* what changes they are requesting when the
- Style changes that the reviewer disagrees with but that are not documented as
Copy link
Member

Choose a reason for hiding this comment

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

I would like to have in the docs what could be a clear order of precedence in terms of styling. As a maintainer of a whole platform sometimes it is necessary to apply tree wide changes that may conflict with other areas. As an example, the styling used for functions may not be the best when you need to change a register.

The intention will be clarify the precedence between SoC, driver, subsystems and libraries areas. For instance, if a vendor maintain the drivers possibly he wants to keep same styling for that drivers. Besides that, people have many experiences and tastes and it is a difficult topic and precedence order on this could help.

This should state too what is the precedence order about maintainer and assignee to solve the conflicts. This could avoid to have people from other areas requesting changing on styling that they should not supposed to ask. I think it is important to write in a way to not create anarchy but to help and avoid conflict resolution.

For instance, what happen when the vendor maintainer open a PR which the assignee or reviewers don't like the style?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Existing code of a unit, if it keeps common style, is documentation of style itself.

There should be some trust in maintainers, as reviewers, that their are not acting in malicious ways, but rather try to represent project and are requesting a change because they need to keep the entire thing in some frames.

Vendors which are maintainers of their support code, i.e. drivers, should have precedence on style over maintainers of the area drivers are in as long as they do not cross the path of subsystem Maintainer. I do not care how, for example, NXP drivers look inside as long as they work correctly in context of API.

If there is a new contributor, creating new file in the area vendor is maintaining, vendor can impose style - because the vendor will be there for longer, represented by on person or another, you can not tell the same thing about a contributor.

part of the project can be pointed out as non-blocking, but cannot constitute
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a link to them?

a reason for a request for changes. The reviewer can optionally correct any
potential inconsistencies in the tree, document the new guidelines or rules,
Comment on lines +310 to +311
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't scale? We need the process to put more work on contributors, not maintainers, within reasons of course. This seems to suggest that we are happy with a PR being merged with stylistic issues/incoherent style and then we want another person to fix it separately.

Copy link
Member

@nashif nashif Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

in a galaxy far away, the following conversation was observed in a PR:

Bob: hey, you are missing a comma there
Alice: Hmm, where is it documented that a comma is needed in this case?
Bob: it is not documented, but the reason I am asking is blah blah blah blah (good reason), I will submit a PR adding this as a new style guideline, would be nice if you do that while we work on the guidelines.
Alice: Sure, makes sense, let me do that, please post the PR adding this as a guideline for reference here please.
Bob: done, PR posted.

possible outcome:

  • PR is merged, everyone is happy.

In another galaxy...

Bob: hey, you are missing a comma there
Alice: Hmm, where is it documented that a comma is needed in this case?
Bob: it is not documented, but the reason I am asking is blah blah blah blah (bad and less convincing reason), I will submit a PR adding this as a new style guideline, would be nice if you do that while we work on the guidelines.
Alice: I do not think your request is justified and I do not think this should be a rule, it is too strict and does not provide any value.
Eve (assignee): Bob, thank you for the review, but your request does not warrant blocking this PR. Please work on a proposal to make this a rule and come back when this was accepted. You can submit a change applying this style once approved if you insist.
Bob: ok, will do.

Possible outcome:

  • PR is merged, Bob submits a PR with a new style guideline, it gets accepted, next time he finds similar issue, his change request sticks.
  • PR is merged, Bob submits a PR with a new style guideline, it is rejected, Bob will never bother contributors with such requests again.

Copy link
Collaborator

@de-nordic de-nordic Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We do assume that existing formatting and style of text of a file may be used as a guideline to enforce?

Copy link
Member

Choose a reason for hiding this comment

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

in a galaxy far away, the following conversation was observed in a PR:

There's also this one:

Bob: hey, you are missing a comma there
Alice: done, thanks
Bob: approved
Obi-Wan: compliance fails. Luke, can you use the force please?
Luke: force merged, thanks

Copy link
Member

@nashif nashif Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There's also this one:

there are many scenarios, too many to list here, obviously this PR intent is to cover the odd cases, not the straightforward ones, your case above can be extended with a different outcome:

Bob: hey, you are missing a comma there
Alice: done, thanks
Bob: approved
Obi-Wan: Why the hell you have a commna, comma is not needed here. -1 from me.
...

or

Bob: hey, you are missing a comma there
Alice: done, thanks
Bob: approved
Obi-Wan: Should be a semicolon, not a comma, -1 from me.
....

Copy link
Member

Choose a reason for hiding this comment

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

And so what? It is red flag for future cooperation anyway. Who wants to work with people that lash out for such a reason, and then drop crayons and storm out of playroom. This is basically taking entire community hostage: no feature if you comment in a way I do not like, and I may not like anything.

When such contributor becomes a maintainer can they keep the attitude or now they are on the other side, and have to agree on anything just to have a new feature?

The above can exactly be said about reviewers, taking the whole project hostage with nitpicking and adhoc/made-up rules that are based on personal preferences.

btw, I did not suggest closing PRs in rage is an accepted or welcome behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This a bit vague but can be interpreted as: style guidelines cannot evolve without a corresponding, comprehensive and mass cleanup (and mass breakage of git blame/revert/cherry-pick). That would be too extreme. Getting consensus on coding styles is hard enough, I don't think the bar should be raised even higher and require a mass cleanup. We can survive with some past inconsistencies as long as the future direction is clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The above can exactly be said about reviewers, taking the whole project hostage with nitpicking and adhoc/made-up rules that are based on personal preferences.

That is true, and some instances that this nitpicking is how a PR can be stalled by reviewer in an attempt to gain time for providing "alternative" solution instead of helping with technical advice to reviewed one.
So yeah, you are right, nevertheless I consider most of our reviewers to be quite reasonable and sane.

btw, I did not suggest closing PRs in rage is an accepted or welcome behavior.

Never implied so, but no guideline against that, I think...

Copy link
Collaborator

@marc-hb marc-hb Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

... optionally correct any potential inconsistencies in the tree ...

Thinking about it a bit more, that very specific bit can simply be dropped (and the rest of the sentence kept as is). That bit sounds like trying to give some "you go and do it yourself first" ammunition to the submitter in the naturally unbalanced "comma power struggle" between maintainer and submitter. Such ammunition is not necessary. I think the answer "there's no rule about these commas so you can't make me" is strong enough alone.

PS: as a submitter, I would likely just add/remove commas as instructed and ask "how high?" I must jump but apparently everyone's different...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Small follow-up submitted:

and then enforce them as part of the review.
Copy link
Member

Choose a reason for hiding this comment

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

"...and then enforce them as part of the review."

Not par of the same review/PR though, or we will end up in a stalemate, PR doing something and another PR introducing guidelines that block the original PR.

So I would change this to..

... and then enforce them as part of the review of future PRs.

Copy link
Collaborator

@de-nordic de-nordic Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No. If contributor does not agree they still are doing the change as requested; after PR gets merged they can then record an issue on style change and propose change to entire unit at once - that is sane way to do that, otherwise you and up with a unit that holds multiple styles and look like crap. That happens, because maintainers stay there and contributors come and go.

What then? Maintainer is supposed to fix it, bring in the "original" style? Use the new one? Then which one? Either way maintainer looses time on something that random person does not agree on but is not willing to put time to change uniformly.
My company will not be happy to pay for me cleaning up after others and people will not be happy to look at things that look like I can not keep the thing tidy.

I have several times people do change and never re-appear. What will happen is that we will have a lot of "style" issues logged in that authors will not follow afterwards, with no will from contributors to follow with change, and leaving code half done.


- Whenever requesting style related changes, reviewers should be able to point
out the corresponding guideline, rule or rationale in the project's
documentation. This does not apply to certain types of requests for changes,
notably those specific to the changes being submitted (e.g. the use of a
particular data structure or the choice of locking primitives).

- Reviewers shall be *clear* about what changes they are requesting when the
"Request Changes" option is used. Requested changes shall be in the scope of
the PR in question and following the contribution and style guidelines of the
project.
project. Furthermore, reviewers must be able to point back to the exact issues
in the PR that triggered a request for changes.

nashif marked this conversation as resolved.
Show resolved Hide resolved
- Reviewers should not request changes for issues which are automatically
caught by CI, as this causes the pull request to remain blocked even after CI
failures have been addressed and may unnecessarily delay it from being merged.

- Reviewers shall not close a PR due to technical or structural disagreement.
If requested changes cannot be resolved within the review process, the
Expand Down
Loading