will I sound crazy if I change my mind here? I thought it would be improving the PR process.. but on a second thought... it won't improve things actually and i will agree with Robbie here.
So. .just ignore me.. and apologize for spamming you guys.. On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish <[email protected]> wrote: > On 04/19/2018 10:32 AM, Robbie Gemmell wrote: >> >> I dont think its unreasonable or unexpected in many cases that a test >> fails to compile without the changes in relates to. It depends what >> type of test it is and what the changes actually are though. >> >> I agree it wont hugely change the PR though. Possibly why I prefer >> them being in the same commit is I tend to use the commit to look over >> things rather than the PR. > > > The other thing to keep in mind is that when two or more commits for the > same bit of work go in, the process of reverting changes becomes more error > prone as the person doing the reverts has to always be looking for the cases > where there was more than one related to the same scope of work. > > >> On 19 April 2018 at 15:10, Clebert Suconic <[email protected]> >> wrote: >>> >>> I have seen a few cases where the test would not compile.. that is the >>> test depends on the changes itself. What is not really Test Driven >>> Development. >>> >>> Both tests and fixes are part of the same PR.. so it doesn't really >>> change much. >>> >>> On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell >>> <[email protected]> wrote: >>>> >>>> In general I think having tests and changes in the same commit is >>>> nicer, especially for looking back at later. >>>> >>>> I'll also often apply a test on its own or revert the non-test changes >>>> to ensure tests fail, I've not really found it slow/annoying enough to >>>> specifically seperate tests out in their own commits to facilitate it. >>>> >>>> Robbie >>>> >>>> On 18 April 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 > > > > -- > Tim Bish > twitter: @tabish121 > blog: http://timbish.blogspot.com/ > -- Clebert Suconic
