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
[1] https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions
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
+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