Thanks Alexey to point this out. I did not know about these 4 points in the
guide. I agree with them also. I would just
add "Avoid keeping in history formatting messages such as checktyle or spotless
fixes"If it is ok, I'll submit a PR to
add this point.Le vendredi 22 mars 2019 à 11:33 +0100, Alexey Romanenko a écrit
:
> Etienne, thanks for bringing this topic.
>
> I think it was already discussed several times before and we have finally
> came to what we have in the current
> Committer guide “Granularity of changes" [1].
> Personally, I completely agree with these 4 rules presented there. The main
> concern is that all committers should
> follow them as well, otherwise we still have sometimes a bunch of small
> commits with inexpressive messages (I believe
> they were added during review process and were not squashed before merging).
>
> In my opinion, the most important rule is that every commit should be atomic
> in terms of added/fixed functionality and
> rolling it back should not break master branch.
>
> [1]
> https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives
>
>
> > On 22 Mar 2019, at 10:16, Etienne Chauchot <echauc...@apache.org> wrote:
> >
> > Hi all,
> > It has already been discussed partially but I would like that we agree on
> > the commit granularity that we want in our
> > history.
> > Some features were squashed to only one commit which seems a bit too
> > granular to me for a big feature.
> > On the contrary I see PRs with very small commits such as "apply spotless"
> > or "fix checkstyle".
> >
> > IMHO I think a good commit size is an isolable portion of a feature such as
> > for ex "implement Read part of Kudu IO"
> > or "reduce concurrency in Test A". Such a granularity allows to isolate
> > problems easily (git bisect for ex) and
> > rollback only a part if necessary.
> > WDYT about:
> > - squashing non meaningful commits such as "apply review comments" (and
> > rather state what they do and group them if
> > needed), or "apply spotless" or "fix checkstyle"
> > - trying to stick to a commit size as described above
> >
> > => and of course update the contribution guide at the end
> > ?
> >
> > Best
> > Etienne
