+1 especially as you’ve explained it will be guidance and not a hard and fast rule.
Sent from my iPhone > On 18 Apr 2018, at 21:06, Clebert Suconic <[email protected]> wrote: > > You can cleanup your commits with a git rebase -i HEAD~<NUMBER-OF-COMMITTS> > > Squash them accordingly... etc... it's an easy operation. > > Complementing what I said earlier... Many committs are ok.. as long as > they are semantically correct... Intermediate committs that you did > along your day should be cleaned up and squashed. that makes reviewing > a lot easier. > > > Even if you are committing directly upstream (you still have that > right as a committer) I would ask you to review your committs before > pushing. We like to have the github PRs as a peer review tool.. and > quite honestly it has stopped even myself from doing damage before. I > like PRs just because of that. > > > > On Wed, Apr 18, 2018 at 3:54 PM, Clebert Suconic > <[email protected]> wrote: >> I would prefer a squash before merging.. or at least some cleanup >> before people submit a PR. >> >> When reviewing a PR.. it's almost impossible to make a correlation >> between the committs... >> >> >> example commit1: added a line, commit 2: removed that same line... it >> would make a PR review useless and more hard. >> >> >> I'm trying to make the review of PRs easier.. having a full day of >> work being sent on a PR as a stream will make the reviewer go through >> what you tried along the day. >> >> >> After all. you should be able to checkout and compile any PR. >> >> >> >> >> Anyways: I'm not asking to add the test separated as a strict rule. if >> for some reason it became difficult to do it along your PR.. that's >> fine. (which this will of course only apply for bug fixes). >> >> >> >> >> >> On Wed, Apr 18, 2018 at 3:34 PM, Michael André Pearce >> <[email protected]> wrote: >>> The only comment here is we will need to stop squashing our commits in >>> general eg be happy a pr may have many commits I’m happy to do this btw, eg >>> internally in our org we don’t care in fact we prefer the full devs commits >>> as then it helps unpick stuff, I found it a shock to be squashing so much >>> when I started. >>> >>> but generally when locally working I may commit multiple times a day >>> different bits eg rework the fix or enhance the test further, it’s one of >>> the big benefits of git for me, but before push and PR I squash the commits >>> as up till now as we don’t seem to like to have this history. >>> >>> >>> >>> Sent from my iPhone >>> >>>> On 18 Apr 2018, at 18:27, Clebert Suconic <[email protected]> >>>> wrote: >>>> >>>> I would appreciate if we separated fixes and tests on Pull Requests. >>>> >>>> >>>> A lot of times i will revert the fixes to validate if the test is good >>>> (if it fails without a fix) and how it failed. (not that I don't trust >>>> the committer, just part of the validation as sometimes I want to see >>>> what was the semantic change and fix). I may eventually play a better >>>> fix in the process.. and I am sure that would apply to anyone else >>>> helping on reviewing commits. >>>> >>>> >>>> I had at some point gone back in history and needed to apply the test >>>> without a fix to find a better fix. >>>> >>>> I know eventually it's not possible to separate these.. but if you >>>> could as much as possible separate them:? >>>> >>>> >>>> I recently did that into PR #2004... >>>> >>>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585 >>>> >>>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585 >>>> >>>> >>>> >>>> >>>> I may update the hacking guide with this.. WDYT? >>>> >>>> >>>> -- >>>> Clebert Suconic >> >> >> >> -- >> Clebert Suconic > > > > -- > Clebert Suconic
