The Crystal Programming Language Forum

Approval and merging process

Hey guys, some of you might have already noticed a delay in merging approved PRs lately. The idea is to ensure more stability and give more chances for additional reviewers to take a look at the proposed changes.

As before, a pull request still needs green CI and two approvals from core team members (one if the author is a member) to be accepted. Once accepted, it is added to the current milestone. But we won’t directly merge it on master. The milestoning signals the PR is scheduled to be merged soon and gives everyone a last chance for comments. The wait time is suppposed to be about a day at minimum but may depend on the gravity of the change. So please don’t ping every core team member 24 hours after the milestone was added. But feel free to bump the PR if a reasonable amount of time has passed without reactions.

This process is a guideline and not a strict rule so there might be occassional exceptions within reason. Urgent infrastructure fixes for example should be expected to skip the line.

8 Likes

BTW, how is the promotion process to Core Members works these days? :slight_smile:
More Core Members can increase development velocity.

If/when new core members get added, I hope they follow the principles and core values set forth by the founders of this language.

This is what Code Guidelines for. Every reputable open source project has them.

Not sure about that. Code guidelines are not really the same as core beliefs instilled into a language by its creators.

Thanks a lot for the communication @straight-shoota, really appreciated :slight_smile:

This explanation or a link to them could be added in a PR template, in order not to discourage contributors, especially new ones.

I can propose a PR in this way. In parallel to this, may I suggest to move forward on https://github.com/crystal-lang/crystal/pull/8934 (please, core members can edit the PR)

I don’t think this matters for the PR template. Let’s not complicate things there. Maybe we could explain the PR lifecycle somewhere (CONTRIBUTING.md ?) and that might be a place to reference this.

1 Like

Essentially the core team looks for candidates and decides who is going to be invited.

It probably has some effect, but I think this is often overrated. You don’t need merge power for making great contributions, reviewing other’s PRs and taking part in discussions. Engaged non-core members can and do actually do a lot of work.
Still helps to have more core team members, but I don’t think we should expect anyone to do more work just because joining the core team.

2 Likes

Ok. Got it. I just felt that sometimes it takes forever to get some core team approvals.
But that is just how I felt when I was working on debugger.

Doing proper reviews is unfortunately an incredibly time-consuming task. Your work on the debugger was amazing! But it resulted in a huge PR with many changes to be reviewed and it progressed over time. It’s also a very specific topic. For example I have a very limited understanding of how all this debugging stuff works, so I couldn’t possibly do a substantial review on that.
Honestly, I was quite a bit surprised by how fast it finally got to be merged. Unfortunately, there have been other precedents where it takes much more time to finish some bigger tasks.
I’m not sure what’s the best way to improve on that. More reviews would certainly help and I might remind everyong that not only core team members can do reviews =)

2 Likes

This may seem like a bad thing, but it’s actually really great. Changes to core APIs, the compiler, etc, require a lot of scrutiny because it has a massive influence on the entire ecosystem and can change how everyone works with Crystal. So I’m glad the core team takes the time to think this stuff through that deeply, even when their scrutiny impedes features I’m pushing to improve my own work.

I frequently have to be that person at work that asks the “what about …” questions to try to avoid shooting ourselves in the foot later, so I have a whole lot of appreciation for not only how important that is but also how thankless. :slightly_smiling_face:

1 Like

I really like thoroughness of the approval process. I also know that core team is very busy with their day jobs, so I thought that more people will help doing it faster.

Would be nice if more open source projects followed this same principle

1 Like