New PR Workflow

[kennethshsu]

Description

Right now, PR and commit history is a bit of a mess

Is your feature request aligned with the scope of the package?

• Yes, absolutely!
• No, but it's still worth discussing.
• N/A (this request is not a codebase enhancement).

Describe the solution you'd like, or your current workaround.

Three things:

1. As discussed in New milestone available: 0.11.0 #1027

Strategy	Keeps commits	Keeps branch structure	Linear history	Clean main branch
Squash & Merge	No	No	Yes	Excellent
Rebase & Merge	Yes	No	Yes	Good
Merge Commit	Yes	Yes	No	Fair

Originally posted by @henrydingliu in #1027 (reply in thread)

I think it's time we reevaluate the merging strategy. I no longer believe Merge Commit is appropriate now that we are larger. Any objections to disable the other two options at the repo level, so only Squash is available?

2. PR status is confusing to look at, is the submitter waiting for a review? Or reviewer waiting for submitter's revision/feedback?

We should add that in the Governing Doc that we turn PRs into draft mode after reviewer has provided feedback. Any objections to this?

3. It's still not super clear who's responsible as a reviewer once PR is opened. Is its just first come first served? Thoughts on how to handle this?

Do you have any additional supporting notes?

No response

Would you be willing to contribute this ticket?

• Yes, absolutely!
• Yes, but I would like some help.
• No.

[genedan]

Any objections to disable the other two options at the repo level, so only Squash is available?

I don't think we should disable the other two options, they all have their place depending on the circumstance. I'm hesitant to do squash merging due to the information loss, and for tasks that might require a long-running feature branch spanning months of work with multiple authors, we should maintain the history rather than compress it into a single commit by one person. In these cases it is certainly possible that changes made prior the current release wind up in a future release - once that was explained as a possibility to the observation made in #1027, I was comfortable with it and I believe it's an accurate reflection of development history.

Squash merging can also interfere with git blame when you're debugging and examining who wrote what lines of code and for what reason, since I believe blame attributes the lines to the person who did the squash.

I think squashing has some uses. The Ruff and Bugbot checks are leading to an increasing amount of minor corrections that don't really need to be their own commits. However, you can also append commits which I think would be the appropriate move if you did something small like fix a typo.

Furthermore, I don't like when my commit messages are wiped out by a squash. I think we should minimize moves that take autonomy and recognition away from the contributors.

PR status is confusing to look at, is the submitter waiting for a review? Or reviewer waiting for submitter's revision/feedback?

The author should put it in draft mode, and then mark it as ready once they're ready for a review.

It's still not super clear who's responsible as a reviewer once PR is opened. Is its just first come first served? Thoughts on how to handle this?

First come first serve works for me, there's only a handful of reviewers. When I see that somebody else starts commenting on a PR, I leave it alone unless I'm mentioned. If I think reviewing might take long enough that somebody else might accidentally start reviewing, I leave a comment that I'm on it.

Regarding these last 2 points, I think if you're looking for some "zero-inbox" zen, I don't think you'll find that in GitHub Issues/PRs, big projects can have thousands of issues and hundreds of open PRs. I would recommend having a zero inbox personal and work email, and then just visiting GitHub a couple times a day to address some outstanding tasks.

As long as we're making good progress in merging in PRs, making issues, and closing issues, we should feel good. Because we live in a world of infinite wants, it's impossible to close out an infinitely long wish-list of new features, performance enhancements, and so on and so forth. That's something we have to accept, but we can find peace in that we moved things forward.

[priyam0k]

Any objections to disable the other two options at the repo level, so only Squash is available?

I agree with @genedan here on not disabling the other options at the repo level. His points on preserving commit history, contributor attribution, and keeping git blame useful on longer branches are really valid.

From my experience, having some flexibility is also helpful when dealing with PR size. If we push too heavily toward tiny stacked PRs, squash merging PR 1 creates new commit SHAs on main that cause messy rebase conflicts for PR 2 sitting on top of it. Managing review order across multiple small dependent PRs also adds extra coordination overhead. For PRs that naturally cover a few related sub-issues (like #1342), keeping distinct commits makes sense.

Maybe a good middle ground is keeping squash as the general preference for simple standalone fixes, but leaving rebase and merge open for stacked or multi-issue branches.

It's still not super clear who's responsible as a reviewer once PR is opened. Is its just first come first served? Thoughts on how to handle this?

From my side as a reviewer, I do find it confusing at times knowing which PR to pick up first from the queue. Currently, I mostly jump into PRs where I am explicitly requested as a reviewer, or ones touching areas I've been working on (like the governing doc or Friedland recreation).

First come first served works when someone has bandwidth, but if have ideas on lightweight ways to prioritize or signal who is reviewing what, I'd be interested to hear what works best.

[henrydingliu]

• on merge type
  • by default, squash automatically populates commit details with the title of each constituent commit. (see here). if we don't want to lose commit details from constituent commits, we can configure the repo accordingly. in practice, i don't see anybody writing any commit details (except bots).
  • squash also fully preserves the author for blaming purposes. here is a commit that I squashed. it's actually better than merge commit, which adds the merger as an author.
  • i don't think we need to disable any of the commit types. i use all three currently. but we should develop general guidelines for commit history hygiene and specific guidelines for when to use which merge type. something like
    • rebase for PR with less than 6 commits and is rebasable
    • merge commit for PR with less than 6 commits but isn't rebasable
    • squash for anything with more than 5 commits
  • this is gonna be immediately relevant as we release 0.10.1, while another dozen PRs are outstanding. How do we make sure none of them distort the changelog for 0.11.0?
• on managing PR status
  • the latest gh feature that lists "last updated" in the PR list should already help a bunch in this regard. each reviewer should assign themselves to the PR as they review. and then @ the raiser after a review is done (in case they turned off notification for general comments).
  • if folks are uncomfortable with immediately turning a qualified review into draft, we can put a timebox on it, e.g. anything that's been reviewed but hasn't been updated for 1/2/3/4 weeks goes back in draft
• on reviewer assignment
  • i've been trying to follow this prioritization ranking. we should discuss, edit, and formalize
    1. issue raiser (e.g. assigning gene to the dims PRs)
    2. recent author (e.g. assigning kenneth to chapter 8 ruff rix).
    3. bandwidth
  • i'd like to prioritize the experience of the contributors, i.e. whoever sees a new PR from a non-maintainer immediately assigns it based on the prioritization we agree on. if the first assignee doesn't have bandwidth, just comment in the PR asking for other reviewers to jump on. we are all very comfortable with each other. don't feel like you can't push back if assigned. this is a hobby we all enjoy, but life and work can take over.

[henrydingliu]

one more thing. we have a little baggage in our little group regarding the gh review feature. i like individual comments because it allows me to break up the review into pieces. gene dislikes the gh review feature since that time he left a review in draft for like a month. the two of us needs to get over ourselves so all of us just use the review feature as gh intends it. then everyone can easily tell which PR has been reviewed.