search

instructlab / dev-docs

Developer documents for the InstructLab organization
Apache License 2.0
4 stars 31 forks source link

Create document that describes merge strategy #7

Closed nathan-weinberg closed 7 months ago

nathan-weinberg commented 7 months ago

We have been doing several different kinds of merges across the instruct-lab repos - we should decide and unifying around something consistent

russellb commented 7 months ago

Maybe a new doc docs/common-github-repo-settings.md where we can add notes on things we think should be common across all (or most) repos.

nathan-weinberg commented 7 months ago

@russellb I will work on creating something like this week!

wking commented 7 months ago

Defaulting to explicit merges gives contributors space to build a change out of several small pivots, and explain each pivot in a distinct commit message. That's likely easier to review, both when the pull is in-flight and post-merge if later developers are looking at git blame ... and trying to figure out what the original developer was thinking.

Building a series of fixups while a pull request is in-flight might make multiple rounds of review easier for a single reviewer who comes back to the same pull for each round of review, and that's great if the contributor and reviewer want to work like that. But the contributor can squash their commits themselves, once the reviewer is happy, into a series of semantic commits or a single commit or whatever they think makes the story most clearly to the hypothetical future developers.

For contributors who are not comfortable doing their own rebase (e.g. a lack of familiarity or access to command-line Git), I think squash-merges make sense. But those can be one-off actions by maintainers reviewing those pulls. It seems unlikely that this style of contribution is frequent enough for it to need to be the default.

russellb commented 7 months ago

It seems unlikely that this style of contribution is frequent enough for it to need to be the default.

Anywhere this is a regular thing just means we need some more senior folks to provide some assistance and coaching on how to do all the things you described: crafting a patch series, the use of git rebase for squashing, ...

nathan-weinberg commented 7 months ago

@wking @russellb I've opened a Pull Request for this issue, let's continue the discussion there: https://github.com/instruct-lab/enhancements/pull/10