doc: contribution guidelines: Clarify and extend #83117
Conversation
carlescufi
requested review from
kartben,
fabiobaltieri,
nordicjm and
nashif
zephyrbot
added
area: Documentation
area: Coding Guidelines
carlescufi
force-pushed
the
contrib-guidelines
branch
2 times, most recently
from
d4bdb82 to
3aa6881
Compare
carlescufi
force-pushed
the
contrib-guidelines
branch
from
3aa6881 to
b4ae559
Compare
|
|
||
| * Use `snake case`_ for code and variables. |
There was a problem hiding this comment.
but this is covered by linux already, in details, I do not think we need to repeat this here:
https://kernel.org/doc/html/latest/process/coding-style.html#naming
There was a problem hiding this comment.
It's not explicit some said, so it was made explicit here.
There was a problem hiding this comment.
I'm in favor of incremental additions to our style guide that eventually make reference the Linux style guide obsolete. While unlikely, Linux can change their style guidelines at any time.
There was a problem hiding this comment.
need to just fork the original linux style into our docs, call them zephyr style guidelines and never look back again.
There was a problem hiding this comment.
kernel.org/doc/html/latest/process/coding-style.html#naming
the more I look at this, the more I think we should just go our own and stop referencing this from Linux. It looks more of a rant than guidelines in some parts.
There was a problem hiding this comment.
kernel.org/doc/html/latest/process/coding-style.html#naming
the more I look at this, the more I think we should just go our own and stop referencing this from Linux. It looks more of a rant than guidelines in some parts.
Yes, totally, and I wanted to point this out the other day. It seriously feels very dated in the way it's written, to the point that it's not even reflecting the values we try to promote via our Code of Conduct, for example... Like, doing something wrong being refered to as being a "shooting offense"... Really?! So yeah if there's something where we can clearly state "we're not Linux" maybe it can start with coding style :) big +1 from me
There was a problem hiding this comment.
Those are very good arguments. I've never before read the Linux coding guidelines like that, but you are absolutely right.
| @@ -133,6 +146,13 @@ PR Requirements | |||
| - Changes to APIs must increment the API version number according to the API | |||
| version rules. | |||
|
|
|||
| - Documentation must be added and/or updated to reflect the changes in the code | |||
There was a problem hiding this comment.
This is already covered and is a duplication, see https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs.
Maybe it is important enough that it should be mentioned twice, my worry is that our guidelines and docs are growing organically and in a negative way, i.e. you might be adding something here that conflicts with somewhere else mentioned in some other part of the docs.
The documentation had for a long time a section that specifically recommends to submit "smaller PRs" for review: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs Yet submitters keep submitting large PRs on a regular basis, sometimes very large ones. See a couple of very recent examples below. (Reminder: submitting a giant, draft PR for pure _testing_ purposes and NOT for review is a perfectly fine) The "natural" explanation is that submitters optimistically and wrongly believe that dumping a large amount of code at once onto reviewers will be dealt with faster than in smaller chunks. This is most likely a contributing factor but most people should quickly learn from bad experience. Yet we keep seeing large PRs on a regular basis. So there must be other factors too. Based on personal but fairly extensive git support experience, another top reason is likely git usability and some lack of git knowledge (neither the first nor the last time git usability would have a significant impact) To help with this, add to the existing git guide the simple command that lets split a large submission in several, smaller PRs. This can only help demystify and promote smaller PRs while barely growing the size of the documentation. While at it, also add a couple missing benefits of smaller PRs. Recent examples of large PRs: - In the controversial and giant PR zephyrproject-rtos#77368 (comment) the exhausted submitter wrote: > Every time any one person requests a rebase that changes the PR, > unless there's consensus, there's no mechanism (manual/project process > or built into GitHub) to know/prevent a different person from rejecting > the new changes. That PR had 21 commits (18 in the final version), 82 files changed and 400 (!) comments. The sheer size massively increased the likelihood of the problem described. Re-submitting it in smaller chunks would obviously have mitigated that problem. Yet that PR was never split and stayed huge...? - In this second example, a large PR was submitted with different authors. A disagreement emerged about squashing across different authors: zephyrproject-rtos#78795 (comment) If this PR had been split in smaller chunks, then the squashing discussion might have been avoided entirely. Whether squashing is good or bad in this particular case is irrelevant (and already discussed at great in length in zephyrproject-rtos#83117). What matters here is: the submitter lost that chance by submitting a larger PR with different authors. Signed-off-by: Marc Herbert <[email protected]>
Clarify and extend some of the PR and contribution guidelines so that they cover practices that have been effectively enforced by maintainers, but were never properly documented. Signed-off-by: Carles Cufi <[email protected]> Signed-off-by: Anas Nashif <[email protected]>
nashif
force-pushed
the
contrib-guidelines
branch
from
beb1b85 to
391f82e
Compare
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]>
| - The PR description must include a summary of the changes and their rationales. | ||
|
|
||
| - All files in the PR must comply with :ref:`Licensing | ||
| Requirements<licensing_requirements>`. | ||
|
|
||
| - Follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`. | ||
| - The code must follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`. |
There was a problem hiding this comment.
"must follow coding guidelines" contradicts what coding guidelines actually say -- the linked page says we "begin enforcement on a limited [and undocumented?] scope" as we're preparing to enter "Stage II", and stage I literally says that PR cannot be blocked when there are violations.
Talked to @nashif and it might be that this whole stage thingy is just obsolete. cc @keith-zephyr
|
@nashif As formulated doesn't this exclude the possibility of multiple authors working on a zephyr PR together? I agree that before merging the relevant commits should be squashed, but should they already be squashed before submitting a PR? |
how does this exclude or prevent collaboration, can you please point to the language you think prevent this? |
|
|
||
| During development, commits may include intermediary changes (e.g., partial implementations, | ||
| temporary files, or debugging code). These should be squashed or rewritten before submitting the | ||
| pull request. Remove non-final artifacts, such as: |
There was a problem hiding this comment.
As partial implementations, ... should be squashed before submitting a zephyr PR, doesn't this exclude the possibility to cooperate on a PR ? Or does this mean that at every stage of development on a zephyr PR the commits should be squashed before submitting the update ?
There was a problem hiding this comment.
you can collaborate in a PR and mark it as a draft, when ready for review, the commits shall follow the guidelines.
There was a problem hiding this comment.
If the initial draft becomes too messy after some time, you can also close it and submit other, cleaner PR(s) to make the final review easier and faster. This is relatively common. Don't forget to link back to the initial one in case some reviewers have time for the long and messy story.
There was a problem hiding this comment.
I think that what could be considered "partial implementation" are fixup type commits. While having some work presented in stages, for not only in draf, the commits should have rather reviewable quality, and should be squashed to one before merge.
For example if you move something around file, or files, and then change the logic, it may aid reviewers to pick these changes in separate commits to see how the move happened, and then diff of the moved code. Having these separate on review makes sense, but on tree does not as the file is tested in CI as this was single-commit change, and probably neither "partial" commit can be removed without voiding that.
On the other hand fixups do not help, because you when you pick one of commits in PR and then there is fixup for some logic, then you have a problem reviewing either commit: one is broken and second is incomplete without the first one, and does not give you full view of a change anyway.
Let me throw here recent PR here #84058, where you can see a potential for two commit, for review time, change, where large block of text is remove and changed at the same time ;).
There was a problem hiding this comment.
@de-nordic renaming and changing at the same time is a common enough use case, not always well handled by git, so maybe it is missing here. I agree it's MUCH easier to review when separate! Both before and AFTER merge.
On the other hand, this PR is clearly insisting on reviewability and there are already recommendations here and elsewhere to have "atomic" commits. So hopefully the risk of misinterpretation is low?
To avoid stalling this PR again, how about a follow-up one from you?
There was a problem hiding this comment.
Renaming and changing at the same time is a common enough use case, not always well handled by git, so maybe it is missing here. I agree it's MUCH easier to review when separate! Both before and AFTER merge.
Small follow-up submitted:
| - When breaking up a PR into multiple commits, each commit must build cleanly. The | ||
| CI system does not enforce this policy, so it is the PR author's | ||
| responsibility to verify. |
There was a problem hiding this comment.
Non-blocking - but this paragraph is now duplicated by the "Retain Bisectability" below.
|
@nashif thanks for following-up on this in my absence! This looks great to me now, hopefully we can merge it soon. |
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. Signed-off-by: Carles Cufi <[email protected]> Signed-off-by: Anas Nashif <[email protected]>
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. (cherry picked from commit ea4e46d) Original-Signed-off-by: Carles Cufi <[email protected]> Original-Signed-off-by: Anas Nashif <[email protected]> GitOrigin-RevId: ea4e46d Cr-Build-Id: 8725452993329915921 Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8725452993329915921 Copybot-Job-Name: zephyr-main-copybot-downstream Change-Id: I7612209e4dfb9d453088919383d32fcd422d5b23 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/6183339 Tested-by: ChromeOS Prod (Robot) <[email protected]> Reviewed-by: Jonathon Murphy <[email protected]> Commit-Queue: Jonathon Murphy <[email protected]>
Clarify and extend some of the PR and contribution guidelines so that they cover practices that have been effectively enforced by maintainers, but were never properly documented.