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
