Sounds good.
I think the high level bit is that whoever merges should *think* about
what they're putting in the history, even if it's just a pausing to
think "should I swash or merge this PR" rather than just clicking the
button.
On Wed, Jul 17, 2019 at 4:59 PM Valentyn Tymofieiev wrote:
>
>
Thanks everyone for the discussion and your thoughts.
Here's my summary:
We don't have to be too prescriptive about who does what and when if we
keep these goals in mind:
1. When a PR is being merged, each commit should clearly do something that
it states, and a commit should do just one thing.
On Wed, Jul 10, 2019 at 5:06 AM Kenneth Knowles wrote:
>
> My opinion: what is important is that we have a policy for what goes into the
> master commit history. This is very simple IMO: each commit should clearly do
> something that it states, and a commit should do just one thing.
Exactly
My opinion: what is important is that we have a policy for what goes into
the master commit history. This is very simple IMO: each commit should
clearly do something that it states, and a commit should do just one thing.
Personally, I don't feel a need to set a rule for who does the squashing
(or
Ok, I think if authors mark fixup commits with "fixup" prefix and
committers routinely fixup commits before the merge without asking the
contributors to do so, the authors should not have a particular reason to
fixup/squash + force-push all changes into one commit after addressing
review comments.
"allow maintainers to edit" by default is enabled. Then the proposed
workflow looks reasonable to me now.
-Rui
On Tue, Jul 9, 2019 at 11:26 AM Kenneth Knowles wrote:
> If you "allow maintainers to edit" the PR, it is easy for any committer to
> fix up the commits and merge. They should not
Rui, committer guide[1] does say that all commits are standalone changes:
We prefer small independent, incremental PRs with descriptive, isolated
> commits. Each commit is a single clear change.
>
However in my opinion, this recommendation applies to moments when a PR is
first sent for review,
Myself usually follows the pattern of "authors force-push their changes
during every review iteration". The reason is after reading [1], I found
it's hard to maintain a multiple commits PR as it's hard to create isolated
commits for different logical pieces of code in practice. Therefore in
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,
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
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
11 matches
Mail list logo
