Thanks Udi, my second question is actually about asking to "unsquash" the change, when doing so will simplify the review process. For example, think of a large PR that received several comments, and author addressed them, however the author squashed all changes into the original commit.
On Mon, Jul 8, 2019 at 11:25 AM Udi Meiri <eh...@google.com> wrote: > I think there are already some guidelines here: > https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives > (maybe > we could point to them from the PR template?) > Yes, it is acceptable to ask for squash or if it's ok to squash to a > single commit. > > On Mon, Jul 8, 2019 at 11:14 AM Valentyn Tymofieiev <valen...@google.com> > wrote: > >> I have observed a pattern where authors force-push their changes during >> every review iteration, so that a pull request always contains one commit. >> This creates the following problems: >> >> 1. It is hard to see what has changed between review iterations. >> 2. Sometimes authors make changes in parts of pull requests that the >> reviewer did not comment on, and such changes may be unnoticed by the >> reviewer. >> 3. After a force-push, comments made by reviewers on earlier commit are >> hard to find. >> >> A better workflow may be to: >> 1. Between review iterations authors push changes in new commit(s), but >> also keep the original commit. >> 2. If a follow-up commit does not constitute a meaningful change of its >> own, it should be prefixed with "fixup: ". >> 3. Once review has finished either: >> - Authors squash fixup commits after all reviewers have approved the PR >> per request of a reviewer. >> - Committers squash fixup commits during merge. >> >> I am curious what thoughts or suggestions others have. In particular: >> 1. Should we document guidelines for iterating on PRs in our contributor >> guide? >> 2. Is it acceptable for a reviewer to ask the author to rebase squashed >> changes that were force-pushed to address review feedback onto their >> original commits to simplify the rest of the review? >> >> Thanks. >> >> Related discussion: >> [1] Committer Guidelines / Hygene before merging PRs >> https://lists.apache.org/thread.html/6d922820d6fc352479f88e5c8737f2c8893ddb706a1e578b50d28948@%3Cdev.beam.apache.org%3E >> >
