The usual flow we have right now regarding pull requests is this:
Someone sends a PR
Core members and the community give feedback, potentially asking changes
From this point forward these things can happen:
The PR author addresses the feedback, the PR looks great and it’s merged
The PR enters a loop of feedback, adjustments and discussions. Eventually the PR becomes stale because the author didn’t have time or will to work on it anymore, or because there’s no clear agreement on how to proceed.
Now, my experience with contributing to Ruby was this:
I reported a bug
I sent a PR to fix it
The PR was merged right away: no comments, no feedback, no discusssions
Immediately after the PR came another commit that adjusted my work to the “core team standards”, potentially addressing things I forgot, formatting things, etc.
As a contributor, I had mixed feelings: it was really nice that my contribution was accepted right away, freeing my from further work, but seeing my work slightly changed felt strange. But… in the end, I think the experience was great! I was able to contribute with minimal effort from my side, and also from the other side (no discussions, no time lost, etc.)
Do you think we should start doing the same thing in Crystal? It could work like this:
Someone sends a PR
Anyone can leave feedback
If we think the feedback can be addressed in a later PR, and there’s time for that PR to arrive before the next release, and if the PR is approved by two core-team members as usual, we merge it
Later on we send a follow up PR to address those changes
The main difference is that the PR author doesn’t need to do anything at all after submitting the initial PR. And the follow-up PR can be done in the exact way that we want the code or comments to be, no need to ask with words and then find out that the code doesn’t match those words.
What do you think? How has been your experience so far as a Crystal contributor?
I think that workflow might work for a less pressured team, in which follow-up PRs might come immediately after. But for us I don’t think it’s sensible at this point (hopefully this will change in the near future). There’s a non-negligible chance a PR will be merged, and the follow-up PR will get in the air.
Also, I think it’s good to give the opportunity to the author to change their own code.
Therefore, I propose a different scheme:
Someone sends a PR
Anyone can leave feedback
Someone from the core team that champions the PR gives the author the choice: (a) address a specific set of changes, or (b) leave it to the team.
4a. Wait for changes, repeat from 2.
4b. A core team member addresses the changes. It’s easy with GitHub’s gh tool to add commits or branch from the PR, so no need for a new PR. Everything is nicely bottled in one PR, easier to see the final state.
I think these approaches both sound good. In @beta-ziliani’s list, 4b sounds like a good workaround for stale PRs.
There’s a few points that I think could be added to clarify things for contributors:
If a second core member approves the PR and it’s targeted for the “next” release, can it be merged right away? That way it has little chance of rotting.
In the case where a core committer or someone from the community is not interested in bringing a PR to the finish line, can it be closed? This would clean up PRs tab but the PR would forever remain searchable. I know sometimes that sends a bad signal to contributors… but I think a succinct PR WIP list sends a stronger signal.
This one is less about PR accounting… but what are the opinions around “short term” fixes? A concrete example is adding a feature to the HTTP client to auto-follow redirects. There’s a long term goal to have a proper middleware layer or some other architecture that would facilitate a whole slew of features. But short term, the problem of redirects never gets solved because it gets stuck behind a major re-architecture. I think having a deliberate discussion around this would be helpful.
Thanks for all these public posts around process lately! It’s really great to have an opportunity to share a thought and hear from others.
While I process 2 and 3 in your list, let me comment briefly:
We let two-members-approved PRs to sit for a day or two to hear from other code members, but then it should be merged. If that doesn’t happen is a mistake.
Ruby’s method: frictionless
Crystals (current) method: frustrating
Both of these lead developers to a 4 letter word (x3). Either “freeing” or “f***”, “f***”, “f***”.
@asterite proposes swinging from one extreme to the other. Hmm.
What was most frustrating and how to remove or improve?
What middle grounds are there?
How to achieve balance with a small team?
Zen
Frustration
Various times I felt the PR was good enough but got stuck behind a series of small changes. The frustration mostly came from (bolded):
Changes requested
Discuss/clarify
Make changes
Wait for review (This part seems much better this year. The entire process is better this year)
REPEAT x5
If all changes were requested at once per core team member the frustration would drop significantly. At least I would have a clear idea of what was needed instead of dreading the next round of reviews.
When to merge / Middle grounds
Is it good enough or willing to delay release until issues are addressed?
Should bug fixes or small improvements (perf, refactoring) be handled different from [API additions, breaking changes, major restructuring]
Other criteria?
Hard outer shell with a soft chewy center. Or maybe the other way
Could core(center) have expanded roles? Reviewers, categorizers, documenters?
Module owners/reviewers. Owners might be too strong. First line reviewers/cleaner uppers for various modules.
Maybe I’m wrong
Ruby worked well, maybe go all the way and reorganization will happen organically
Unlikely ideas
Master becomes open development branch (or new develop branch) with cherry pick
Release happen in release branch cherry picking commits.
Scratch this idea, it seems worse and more error prone than working on PR’s.
When would a forgotten commit get merged? How would they be tracked? Discussions? Additions?
Master becomes development branch with repeat merging of PR branches
Only benefit is discussions/commits are documented betterd
How can I take over a stale PR? Specifically #6459 which I think is stuck on some minor documentation changes. The exact sort of thing addressed in this post.
I think that PR’s should get to the place where they’re entirely acceptable to get merged, then they should be merged. If a maintainer needs to add a few changes, they can add changes directly to the PR before merging to prevent “potentially bad code” from getting into the main branch.
Along with this, I also believe that contributing would have less friction if force pushes on open PRs were allowed, which matches a lot of people’s workflows for PRs in the wild.
As a recovering force push user I have to back the Crystal Team even through I originally wanted to force push. With incremental changes the reviews are faster. Why? Because they don’t have to start from scratch after every force push.
From a security POV force pushes terribly hard to handle. It’s much easier to sneak something through because the reviewer(s) may stop reading carefully after a while.
At work, the reviewer can modify the branch he is reviewing.
This led to less time spent with the reviews, and fewer repetitions.
For small changes, it’s sometimes faster to change it instead of writing to change things.
For bigger changes, I think a follow-up PR is the best way to handle it.
I don’t think these changes count as documentation really (but I agree they’re minor). The reason that PR got stalled is because the author showed interest in fixing it, but then didn’t (I’m not putting the blame, they already did a lot of the requested changes!).
I see two options: either create a new PR based on that one, or wait for us to fix it.
The problem I see here is that I feel uncomfortable modifying other’s people’s code without permission. Feels a bit rude. But perhaps it’s just a matter of making it clear that this might happen.
My opinion is that not all PRs are stuck because of requests for follow-ups; sometimes it is simply because the diff is deemed too large. For example one may want to separate refactors from a bug fix PR, but the refactors themselves don’t have much utility outside the bug fix’s context. Here merging the PR right away won’t lead to any follow-up actions at all, whereas not addressing the feedback implies the PR cannot be merged.
The time involved matter a lot there, for me. I wouldn’t pick up stewardship of something someone else has worked on during the last months or so, but if the issue champion has not worked on it for years (as is the case if the issue is from 2018), then I don’t see how it isn’t a good thing for all involved that someone pick it up and push it to finish.
In unclear cases I’d suggest to put forth a question in the issue if the author is ok with me picking it up.
+1 on @beta-ziliani proposal, just make clear on stalled PRs that some core team may do a take over on the PR and at the end add a Co-authored stamp in the commit.
Maybe a bot can write a message to new PRs, something like “Any core team member can modify and be co-author of this PR after X days of inactivity.”
The thing that kind of confuses me is you get a PR “accepted” and then nothing happens…because some release is impending. Would be nice to maybe leave a comment “plan to merge this after we release 1.x” or something…
The real drawback for the PR submitter is nit picking results in a lot of pain somehow…for me if there are nits, go ahead and make them and still commit the PR…far easier… :)
I don’t get that. Why does amending new commits cause more friction than rebasing and force push? I actually feel quite different about it (not just from a reviewer’s perspective, but also as a PR author).
Even if there’s less friction for authors, I’m convinced that force-pushes cause a lot more friction for reviewers.
Accepted PRs get attached to a milestone. That’s supposed to signal that the PR is going to be merged. (see contributing guide).
Maybe that’s not clear enough? Should we add a comment alongside the milestone to explains this?
(I mean it)