doc: contribute: Extend Reviewer Expectations with additional rules #83176
Conversation
carlescufi
requested review from
kartben and
nashif
and removed request for
kartben
| - Whenever requesting changes, reviewers must be able to point out the | ||
| corresponding guideline, rule or rationale in the project's documentation. |
There was a problem hiding this comment.
*including past rulings made at architecture or TSC meetings
There was a problem hiding this comment.
Past rulings should not be included:
- They are not readily available,
- They are specific to a case and cannot be generalized without review of the rule,
Past rulings should undergo a review before being added to the documentation as a general rule, this allows a broader audience to verify that this general rule can be applied in all cases.
There was a problem hiding this comment.
If a past ruling applies to one specific thing like "this new subsystem is allowed" then it will say that in the minutes, if it applies to all rulings like "commits must be squashed properly" then it should apply to all, if someone is going to request changes then it is up to them to provide a link and excerpt from the minutes to substantiate that, and the architecture/TSG rulings are binding, as per their project roles, so yes this absolutely should remain here
There was a problem hiding this comment.
Everything is changing, past rulings can be revoked, no longer be applicable or detailed further. Whatever ruling that has been set in the past could no longer be valid. When a new subsystem is allowed it could later down the road be removed, the same is valid for general rulings. "General" rulings should be added to the documentation and removed or detailed when needed. "Specific" rulings (this new subsystem is allowed) should stay specific and cannot be used as a generalized ruling.
There was a problem hiding this comment.
past decisions should be documented somewhere when the decision was made. An exception made at one point does not mean it becomes a rule unless it is also documented. Maybe provide some examples of such "past rulings" and look into what those are not doucmented guidelines yet.
| potential inconsistencies in the tree, document the new guidelines or rules, | ||
| and then enforce them as part of the review. | ||
|
|
||
| - Whenever requesting changes, reviewers must be able to point out the |
There was a problem hiding this comment.
Shouldn't this say "style-related changes"? I don't see how we could ever provide guidelines for every kind of change that a reviewer/maintainer might request, especially technical ones, design/architecture-related, choice of algorithms, etc.
There was a problem hiding this comment.
yes, agree. Need to distinguish between change requests about the code, design and and logic vs how they were written and the style used.
|
|
||
| - Whenever requesting changes, reviewers must be able to point out the | ||
| corresponding guideline, rule or rationale in the project's documentation. | ||
| This does evidently not apply to certain types of requests for changes, |
There was a problem hiding this comment.
"evidently" is a strange choice of word in such a formal document. If there is evidence it should make an explicit reference to it.
| @@ -305,10 +305,23 @@ 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. | |||
|
|
|||
| - Style changes that the reviewer disagrees with but that are not documented as | |||
| part of the project can be pointed out as non-blocking, but cannot constitute | |||
There was a problem hiding this comment.
Maybe add a link to them?
| @@ -305,10 +305,23 @@ 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. | |||
|
|
|||
| - Style changes that the reviewer disagrees with but that are not documented as | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Updating assignee and adding label as a way to make sure this doesn't get rushed. Can we agree that this can wait until the new year to give people enough time to provide input? This, just like #83117, is essentially an RFC, not just a regular PR, so we need to let people time to provide the "C"s :) |
| part of the project can be pointed out as non-blocking, but cannot constitute | ||
| a reason for a request for changes. The reviewer can optionally correct any | ||
| potential inconsistencies in the tree, document the new guidelines or rules, | ||
| and then enforce them as part of the review. |
There was a problem hiding this comment.
"...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.
There was a problem hiding this comment.
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.
|
need to have some language re escalation and dismissal of change requests that are not supported by any guideline, i.e. style changes. We want to have a short feedback loop and quick resolution of such disagreements, i.e. we want to avoid having those go to multiple meetings and forums and try as much as possible to resolve them in the PR. |
| a reason for a request for changes. The reviewer can optionally correct any | ||
| potential inconsistencies in the tree, document the new guidelines or rules, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We do assume that existing formatting and style of text of a file may be used as a guideline to enforce?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
....
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
... 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...
There was a problem hiding this comment.
Small follow-up submitted:
nashif
force-pushed
the
review-guidelines
branch
from
4d8addf to
cfd5240
Compare
| 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 |
There was a problem hiding this comment.
| - Reviewers shall be *clear* about what changes they are requesting when the | |
| - Reviewers shall be *clear* about what changes they are requesting when the |
Apparently had a stray leading space in my request for change sorry about that @nashif
This change was triggered by a review comment linked below: zephyrproject-rtos#83117 (comment) It extends the current Reviewer Expectations with additional rules agreed upon by multiple Zephyr contributors in order to simplify and standardize the decision-making process during PR reviews. Signed-off-by: Carles Cufi <[email protected]> Signed-off-by: Anas Nashif <[email protected]>
nashif
force-pushed
the
review-guidelines
branch
from
cfd5240 to
cb97da0
Compare
This change was triggered by a review comment linked below: #83117 (comment)
It extends the current Reviewer Expectations with additional rules agreed upon by multiple Zephyr contributors in order to simplify and standardize the decision-making process during PR reviews.