Hi!
Ludwig recently suggested to add a rule to the coding conventions [1] to forbid editing commits in an open Pull Request that is still under review. I support this idea because otherwise it is difficult to keep track of the development of the PR and integration of comments.
Any objections?
Cheers, Oleg
Hi Oleg,
otherwise it is difficult to keep track of the development of the PR and integration of comments.
Can you provided more information about what situation do you have in mind?
- That an ACK gets invalid on update - that is visualized by github... the ACK is in the time line before the commit - needs to get reACKed
- That you can't track changes to PRs? - In case there was a problem you did a node - node gets removed by github gui an change of the line (with a forces update) - go to case one (for reACKing)
Best Christian
Hi Christian,
>otherwise it is difficult to keep track of the >development of the PR and integration of comments.
Can you provided more information about what situation do you have in mind?
- Developer A starts a PR for branch N with additional commit X. - Developer B reviews and suggests an enhancement. - Developer A integrates this enhancement with commit Y.
If commit X and Y are squashed (or A ammends X), it can be become annoying to find the actual change regarding the comment from developer B. The reviewer cannot differentiate between the stuff already reviewed and the new stuff.
The proposal would add commit Y to N until the review is completed. After the final review X and Y might get squashed.
Cheers, Oleg
Hi,
+1
the real task: make this understandable for newbies...
Christian
Hi,
I'd add this paragraph to the Development Procedures [2]:
""" 9. When updating a pull request, push the changes as individual commits so the reviewers can tell how their comments have been addressed. (I.e. Refrain from squashing/amending changes to the pull request immediately.)
E.g.: "address comment by @UserName - fix license header comment URL"
Once the pull request gets acknowledged, these follow up commits should be squashed in a reasonable manner before merging. """
Cheers, Ludwig
[2] https://github.com/RIOT-OS/RIOT/wiki/Development-procedures
P.S.: Probably reordering/renumbering/merging into a prior point/... would make sense.
Hi!
I would suggest to change the following
"Once the pull request gets acknowledged, these follow up commits should be squashed in a reasonable manner before merging."
to
"Once the pull request gets acknowledged, theses follow up commits can be squashed in a reasonable manner before merging."
Cheers, Oleg
I would suggest changing "theses" to "these" again but agree otherwise 
Cheers, Ludwig