Squash/Merge PRs

2018-07-12 Thread Naveen Swamy
Hi All, I am seeing that maintainers merge PRs into the repo, they are squashing the commits in the PR, which I understand and agree is to keep a sane commit history, however this is causing problem when there are multiple contributors involved on a PR(by contributing to a fork of the repo) this e

Re: Squash/Merge PRs

2018-07-12 Thread Marco de Abreu
Hi Naveen, I'm in favour of the squashing, considering the number of commits in some PRs and especially because of some people making commit messages a la "fix" "fix" "fix" all the time. Additionally, it gets hard (not impossible, just more inconvenient) to determine the atomic states of master -

Re: Squash/Merge PRs

2018-07-12 Thread Hao Jin
+1 for Marco's proposal on using the co-author field. IMHO the usage of co-author field should not be that often, consider: 1) If a PR is so big that it needs multiple people to contribute a substantial part of it, why can't that PR be broken down into smaller PRs each submitted by single one of th

Re: Squash/Merge PRs

2018-07-12 Thread Anton Chernov
Unfortunately there has been significant push back for small iterative PR's and as a result they have grown substantially involving multiple contributors, multiple commits, sometimes multiple branches to be worked on. I fully agree and support all points that Jin raised: 1) Most contributions sho

Re: Squash/Merge PRs

2018-07-12 Thread Aaron Markham
This is a great discussion, and close to my heart, because I want to include more writers and editors in our community, and I'm struggling to see how to best manage this. It seems like being the sole contributor on a feature is like an engineer's Lone Ranger badge of pride! I feel like it should b

Re: Squash/Merge PRs

2018-07-12 Thread Hao Jin
Hi Aaron, To be fair, this discussion has nothing to do with "pride" of SDEs, but rather a discussion on what is a better software engineering practice for the project. Breaking a large project into smaller tasks is a good software engineering practice. This article(https://arxiv.org/pdf/1702.01715

Re: Squash/Merge PRs

2018-07-12 Thread Aaron Markham
My point was about collaboration, or the lack thereof, and how the tooling and apparent rewards from contribution statistics can reinforce lone ranger behavior. People can and should be proud of their work, but why does it have to be alone? Working within the context of a team is something to be pr

Re: Squash/Merge PRs

2018-07-12 Thread Marco de Abreu
As of now it will require the usage of a merge bot as far as I understand this issue: https://github.com/python/miss-islington/issues/16. Instead of using the web interface do merge, we'd then trigger the bot to do the merge on our behalf. There are pre-made solutions on the internet we might be ab

Re: Squash/Merge PRs

2018-07-12 Thread Naveen Swamy
@Aaron, I do not think most contributors(SDE or not) were even aware that their commits are getting squashed into one and thereby others losing credit on that work. I would assume no bad intentions there. @Hao, Agree to breaking down into small and reasonable sized PRs, but I think very small PRs

Re: Squash/Merge PRs

2018-07-12 Thread Pedro Larroy
This is a great discussion, thanks for raising the question Naveen. >From my experience working in all kinds of software project of varying sizes and flavours: 1. People should be aware of git rebase interactive and have more hygiene in their PRs. If a PR is hygienic and is separated in a s

Re: Squash/Merge PRs

2018-07-12 Thread Marco de Abreu
Fully agree, if we can get our commit hygiene up to industry standard, I don't see any problems in using rebase merge instead. For the short term I agree that it should be fine to rebase merge individual PRs with multiple contributors. But in my opinion we should then insist on people amending thei

Re: Squash/Merge PRs

2018-07-12 Thread Naveen Swamy
Yes to insist on commit hygiene when rebase-merge. We have to open a JIRA with Apache Infra to enable rebase-merge on the repo. On Thu, Jul 12, 2018 at 3:21 PM, Marco de Abreu < marco.g.ab...@googlemail.com.invalid> wrote: > Fully agree, if we can get our commit hygiene up to industry standard, I

Re: Squash/Merge PRs

2018-07-12 Thread Marco de Abreu
Coukd you elaborate why we would need a ticket for that? GitHub supports it out of the box and it is enabled as far as I can tell. You just have to press the little arrow besides the merge button. -marco Naveen Swamy schrieb am Fr., 13. Juli 2018, 00:54: > Yes to insist on commit hygiene when r

Re: Squash/Merge PRs

2018-07-12 Thread Naveen Swamy
You are right, its already enabled :) I saw that option greyed out for one of the PR(for a different reason) and assumed we need INFRA to enable. On Thu, Jul 12, 2018 at 4:22 PM, Marco de Abreu < marco.g.ab...@googlemail.com.invalid> wrote: > Coukd you elaborate why we would need a ticket for tha

Re: Squash/Merge PRs

2018-07-12 Thread Marco de Abreu
Excellent :) I don't think we need a formal vote for this but rather let this be lazy consensus if nobody minds. Could we maybe revisit this decision in 1 or 2 months and then assess the state of commit history in all PRs (including squashed ones) and with focus on rebase merged PRs? -Marco Nav

Re: Squash/Merge PRs

2018-07-12 Thread eric xie
-1 We should stick with always using squash. It maintains PR reference in commit history. It is also common practice. If you want to included commits from multiple contributors in a single PR, open a branch and make PRs to that branch. Then only use rebase when merging that branch to master.

Re: Squash/Merge PRs

2018-07-13 Thread Aaron Markham
@naveen - Hanlon's razor applies, so I am not concerned with any individual's action or inaction here. Most people don't even realize its happening (I didn't until recently), and the common practice doesn't manage collaboration well at all. Let's fix that, and turn a less-than-ideal common practice

Re: Squash/Merge PRs

2018-07-13 Thread Steffen Rochel
Thanks Aaron for restarting the discussion. A vote or decision about voting should not be called in the middle of a discussion either. It is not reasonable that everybody can follow every discussion. A vote should be a separate thread with [VOTE] in the subject and concise description what is the v

Re: Squash/Merge PRs

2018-07-13 Thread Eric Xie
Yes that's what I'm suggesting. One important reason why squashing is common practice is that it guarantees that all commits in master has passed CI. This makes it much easier to use bisecting to find bugs. Thanks, Eric On 2018/07/13 14:02:49, Aaron Markham wrote: > @naveen - Hanlon's razor