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 >
smime.p7s
Description: S/MIME Cryptographic Signature
