The current protocol for “submitting, reviewing, and merging” PR’s feels really confusing.
I would expect something like “you submit a PR. After 3 approvals, it is merged within…some time frame”
The current process feels so confusing.
There are PR’s with approvals that aren’t merged.
There are PR’s that got feedback, the feedback was responded to, then silence.
Bug reports where questions were answered and then silence.
It’s feels confusing.
Brainstorming, my first thought here was to create a forum thread called “request for review” or something where we could ask for attention to a particular matter (PR, merge request, etc.)
Maybe at least a document somewhere describing what should happen, and what to do when it seems to not be happening?
The current protocol is essentially “You submit a PR. After two approvals from core members it gets merged.”
I’m not sure what’s confusing about that.
If there are unmerged PRs with two approvals, please point them out. Might have just been forgotten. AFAIK there’s no search filter on Github for that (maybe there’s a bot or other kind of API integration?).
Core members’ review capacity is unfortunately a limited resource. We try to allocate time for contribution reviews and do them thoroughly. When there’s much traffic, it can easily happen that discussions waiting on feedback slip through. Feel free to ping such PRs when you think they’re ready for another round of review.
I don’t see how that’s related. When questions are answered to make the bug report fully comprehensible, then it’s just a complete bug report, waiting for someone with time and motivation to fix it.
Another potential source of confusion for both reviewers and authors is that if after reviews sustancial changes are made the prior reviews does not count any more.
It might seems that the merging criteria is satisfied but it’s not.
Core members that haven’t review might asume original reviewers will re-review.
Original reviewers might have lost that a re-review might be expected.
And flood nudging should be phased to avoid more notification noise.
But if stalled status seems to be reached a heads up is welcomed.
Big thanks to RX14 and straight-shoota for merging some of my PR’s recently
Yeah something in github UI is confusing, maybe we can just blame it all on github
If you look at the list of all PR’s:
there are several that say approved but that haven’t been merged.
Maybe they could be tagged with something to help like “needs work”?
It’s not immediately clear if leaving a comment on a PR will send a message to “the entire core team” or just to the ones that have participated (much fewer, and “is it enough to get the approvals needed?” it’s unclear), so it leaves authors confused, not quite sure what to do next to get more attention. And begging feels awkward…or begging for a final decision when there’s some indecision (ex: “we’ve decided to just not do this this way…” we just want a decision one way or the other). It kind of feels like there’s no real ownership for merges like…nobody knows who’s supposed to actually do the merge, plenty of core people drop by and “approve” but then somehow nothing happens…maybe a new tag couldn’t be added “ready to merge” or something?
I still think a long-running thread here on the forum might go a long way for being able to bring some visibility to issues or PR’s where attention is lacking, in an understandable way, thoughts?
Before submitting a PR, make sure to pack your bags and head to the nearest tornado shelter. Then, embrace the storm, and after the PR is done (merged, closed, etc), repent for your sins. This is the key to becoming the best open source contributor in the world.
It’s just GitHub saying it’s approved. This already happens when there is only a single approval (but we require two for merge) and might also include outdated reviews when the PR received substantial changes afterwards.
I always look at all comments by having the list of issues/PRs sort:updated-desc. So I also see updates to discussions I’m not subscribed to. At the current rate of traffic this is usually manageable. I’m not sure about the other core members but I suppose it worka similarly.
Again, if such PRs exist, please notify!
As far as I can tell, this usually works pretty well. The second approver typically performs the merge. There might be exceptions when we feel some extra sets of eyes would be great, especially when a core member familiar with the code hasn’t voiced yet (usually listed as pending review).
In other cases the PR might be accepted but not merged right away simply for procedural reasons, for example CI hasn’t finished or we have a temporary merge stop at release time. In that case, the appropriate milestone gets added to signal it has been accepted.
OK I will go ahead and post a comment to a PR. if no response in 3 days I’ll post a comment here on the forum.
It is definitely confusing that github says “approved” on them with only one approval (i.e. not yet really approved)… :|
For issues where “yep that’s a bug” but “nope we’re not going to address it immediately” might be nice to slap a tag on there to let them know what the status is, like “help-wanted” or “help-or-pr-wanted” or even “help-or-pr-or-funding-wanted” so they know it’s not being worked on actively, but isn’t being ignored? Just an idea there.
Also might be nice to do a periodic “bug scrub” (once/year?) and check if submitters are still pursuing PR’s and issues or if they should just be closed.
Cleanups. Cheers!
Regarding periodic “bug scrub” I think everyone in community is welcome to do it. I did it once, which resulted is some no longer valid bugs being closed.
If you like Crystal and want it to become even better anything like closing old issues will help. Even if it’s not the most exciting task to do like triaging the old issues.
