Squash PR-s

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Squash PR-s

Aleksandr Mashchenko
Hi,

Right now some PR-s have quit large number of commits. Even PR-s that
fix pretty trivial issues tend to grow fast (test fixes, improving stuff
that came up in the review, etc.)

That makes it harder to pinpoint exact commit that addressed issue at hand.

There is a Squash and merge option [1] on the GitHub for PR-s. I suggest
we should start to use this option along with the common sense. (For
example, adding improvement in one commit and committing tests for it in
another is probably fine in most cases.)

Some random discussions about squashing:

https://github.com/SatelliteQE/robottelo/issues/3803
https://github.com/reenhanced/gitreflow/issues/52


[1]
https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits

---
Regards,
Aleksandr

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Squash PR-s

Yasser Zamani


On 11/7/2017 12:16 AM, Aleksandr Mashchenko wrote:
> I suggest we should start to use this option along with the common sense.

I agree. Good for cases such [1], however, not good for [2] for example
where commit 1 and 2 should not be squashed as I intentionally separated
them to enable easier track of changes and concepts.

[1] https://github.com/apache/struts/pull/173
[2] https://github.com/apache/struts/pull/167

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Squash PR-s

Christoph.Nenning
In reply to this post by Aleksandr Mashchenko
let's see how much we agree what common sense is :)


regards,
Christoph



On 11/7/2017 12:16 AM, Aleksandr Mashchenko wrote:
> I suggest we should start to use this option along with the common sense.

I agree. Good for cases such [1], however, not good for [2] for example
where commit 1 and 2 should not be squashed as I intentionally separated
them to enable easier track of changes and concepts.

[1] https://github.com/apache/struts/pull/173
[2] https://github.com/apache/struts/pull/167

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Squash PR-s

Lukasz Lenart
In reply to this post by Yasser Zamani
2017-11-07 7:17 GMT+01:00 Yasser Zamani <[hidden email]>:
> I agree. Good for cases such [1], however, not good for [2] for example
> where commit 1 and 2 should not be squashed as I intentionally separated
> them to enable easier track of changes and concepts.
>
> [1] https://github.com/apache/struts/pull/173
> [2] https://github.com/apache/struts/pull/167

Yeah... it's not always a good idea to Squash commits, I think we need
a common sense and explicitly say that this PR can be squashed instead
of merged. We can use a label or type in directly in the description,
something like "Please squash this PR"


Regards
--
Ɓukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Squash PR-s

Yasser Zamani-2


On 11/15/2017 1:26 PM, Lukasz Lenart wrote:
> We can use a label or type in directly in the description,
> something like "Please squash this PR"

Yes, I think as it rarely happens, maybe a new github label is not
needed. Instead, when merger feels a squash suitable case then discusses
pull requester to squash which commits.

I also think we should not squash to only decrease commit counts.
Instead, we should honor "easy to track how and why the changes are
made, via enough modular commits counts, not more, not less". When
squash helps this, then we do it.

Regards,
Yasser.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]