Merging Hygiene

[mrocklin]

How do we feel about git hygiene when merging? I personally tend to squash-and-merge PRs, except if git history is particularly clean (this happens very rarely for me). Any thoughts?

When people maintain larger projects it's usually nice to be able to assume a few things about git history

1. It is entirely linear, without branching
2. All tests pass on all commits

These conditions are very rarely held during normal active development of any feature branch, but are typically held when a branch is merged. We can use github's squash-and-merge feature to automatically squash all commits in a PR to one. This loses some history though, so there is a balance here if the git history of that branch is particularly valuable.

Most pydata projects that I know of use squash-and-merge by default.

[CJ-Wright]

squash-and-merge works for me. I personally am often guilty of committing breaking code usually as a way to keep track of where I'm at in the middle of a refactor or so I can have other people look at the code, so I'm usually less strict about it, but I could try to be cleaner about that. I guess with squash-and-merge we are guaranteed that all tests pass so long as travis passes on the last commit so I am ok with that.

[mrocklin]

Committing breaking or WIP code makes lots of sense. I do it all the time. Squash-and-merge allows this kind of workflow during a PR without having it bleed into the final history.

[CJ-Wright]

Do we want a .github/contributing.md to go along with this? (Can we just lift one off of another pydata project?) Edit: formatting

[mrocklin]

I see that as separate. That is information that a newcomer should be aware of like write tests, check style, document functions, etc.. This issue is about how people with commit access should manage things.

On Tue, Sep 5, 2017 at 1:04 PM, Christopher J. Wright < notifications@github.com> wrote:

Do we want a .github/contributing.md to go along with this? (Can we just lift one off of another pydata project?)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mrocklin/streamz/issues/61#issuecomment-327240959, or mute the thread https://github.com/notifications/unsubscribe-auth/AASszLzJWrddXU0Zim1mEqQ5Iq5w7-0Zks5sfX87gaJpZM4PNN9X .

[CJ-Wright]

That's fair (maybe github should have an advanced contributing guide). Do you want to enshrine this information somewhere? If so, where?

[mrocklin]

In dask I tend to just informally tell individuals when they get commit rights. Happy with whatever

On Tue, Sep 5, 2017 at 1:12 PM, Christopher J. Wright < notifications@github.com> wrote:

That's fair (maybe github should have an advanced contributing guide). Do you want to enshrine this information somewhere? If so, where?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mrocklin/streamz/issues/61#issuecomment-327243147, or mute the thread https://github.com/notifications/unsubscribe-auth/AASszCk_oUYrImEj9A0GF22vgPqqPJEwks5sfYDzgaJpZM4PNN9X .

[jrmlhermitte]

I am fine with whichever. Personally, I do like micro commits as they could be useful for code forensics. But the PR's can also serve this (assuming that we can still see comments on outdated diffs after the history has been modified?).

[jrmlhermitte]

Perhaps we should have a suggestion message on the commits, with rebase instructions, such as:

To squash your commits, please type: git rebase -i HEAD~N where N is the number of commits you want to squash. You will be shown a list of commits. Please replace pick with squash with each commit except the latest. When finished, you will be prompted for a commit message. Please write as detailed (but terse) of a commit as you can. Remember to keep the first line short, and add the longer details two lines below. When finished, you will have to force push to your branch: git push origin mybranch -f where origin is the alias to your remove and mybranch is your branch.

Also, tried to conform with discussion here in #66

[mrocklin]

I don't think we need to impose this on contributors. Instead, when merging, we can select the "Squash and Merge" option for the green button. This will do the squashing automatically.

On Thu, Sep 28, 2017 at 11:36 AM, Julien Lhermitte <notifications@github.com

wrote:

Perhaps we should have a suggestion message on the commits, with rebase instructions, such as:

To squash your commits, please type: git rebase -i HEAD~N where N is the number of commits you want to squash. You will be shown a list of commits. Please replace pick with squash with each commit except the latest. When finished, you will be prompted for a commit message. Please write as detailed (but terse) of a commit as you can. Remember to keep the first line short, and add the longer details two lines below. When finished, you will have to force push to your branch: git push origin mybranch -f where origin is the alias to your remove and mybranch is your branch.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mrocklin/streamz/issues/61#issuecomment-332875569, or mute the thread https://github.com/notifications/unsubscribe-auth/AASszG9-rO4CGztzfxWl23mQmo3tptLiks5sm7zqgaJpZM4PNN9X .

[jrmlhermitte]

thanks, I was not aware of this!

[dhirschfeld]

@jrmlhermitte - that level of git experience is so far above most analysts that you'd be excluding a lot of people and introducing a huge barrier to contribution. As @mrocklin says the GitHub UI can do that for you and that can even be enforced by the repo settings.

[CJ-Wright]

I think this issue was really aimed at all those who have merging privileges, since we can do the squash via github and its our responsibility to make sure that the history is as we expect it.

[jrmlhermitte]

so we've been squashing and merging. Should we write up some developer documentation to close this?