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 pu
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 A
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, Ju
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
practic
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, an
At least for me, because I usually don't know when PR review is done, in
order to make PR to be merged into Beam repo faster, I keep squashing
commits every time so that committers can review and then merge at a time,
otherwise committers could approve a PR but then ask squashing commits,
which lea
If you "allow maintainers to edit" the PR, it is easy for any committer to
fix up the commits and merge. They should not have to ask you to do it,
unless it is not obvious what to do.
Kenn
On Tue, Jul 9, 2019 at 11:05 AM Rui Wang wrote:
> At least for me, because I usually don't know when PR re
"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 hav
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.
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 n
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 how
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.
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:
>
> Tha
13 matches
Mail list logo
