Committer Merge Policy #3157
Comments
|
👍 Maybe not require a like for additions to the internal package that otherwise doesn't affect existing code or is essentially unused (as of the time). Example: addition of a new queue implementation for later use, bugfix to this queue when it is still not in use. Deleting unused code still requires a like. Perhaps it is better to state that PRs requiring likes with at least 2 likes can be merged, since the merge can be performed by either of the like giver or somebody else. |
|
👍 |
|
👍 In addition, I think the build script changes also require a like, such as #3089. |
|
Agreed @zsxwing |
|
Added "changes to build or release" to description above. |
|
I've been thinking about this and I'd add a timeout to the second approval. How about 3 business days after which if nobody else complains, the first and only approver can merge stuff on his own responsibility? |
|
+1 to this, I guess you'll receive response to the comment in 3 business days :) But I'd keep "at least 2 👍s" rule via requiring 👍 from one or more developers who are not part of the RxJava GitHub team but have at least 1-2 good contributions to the RxJava. |
|
@artem-zinnatullin does raise a point of interest to me. I've contributed about 4000 lines to the code base mostly in the last year but as a non-committer would not be expected to put a thumbs-up on issues/PRs but rather something like LGTM? |
👍 for RxJava team and LGTM for others looks good to me. More eyes and feedback — better result :) |
|
I don't see why the distinction is needed or useful. Also why are all of the v2 PRs being merged without any review? On Thu, Sep 3, 2015, 6:13 PM Artem Zinnatullin [email protected]
|
Just found that GitHub displays "collaborator" and "owner" near to the comments, hadn't seen it for years… Now I totally agree, distinction is not needed at all.
This and PRs without tests in v2 bother me too, though the speed of v2 development is fantastic. |
|
The speed is too fast to keep up! But that's a good complaint to have. On Thu, Sep 3, 2015, 7:12 PM Artem Zinnatullin [email protected]
|
|
The process with v2 was to reimplement operators first then port the unit tests from v1 because they depend on multiple operators most of the time. Besides, reviewing hundreds of operators would take months at the current review speed due to the +2 rule. |
|
Your first sentence explains the how, not the why. I still don't see why So I don't understand why this merge policy exists if it only applies when On Thu, Sep 3, 2015, 7:39 PM David Karnok [email protected] wrote:
|
|
No point in reviewing till unit tests in and passing and all of this action is on the 2.x branch. Might have to wait till a big chunk goes in then review? Can I suggest that all PRs associated with v2 have a title prefixed with v2? That way I can filter them mentally when I look at email notifications from github or the abbreviated github combination issues/pr view. I realize that these PRs have been assigned as Milestone 2.0 but that is only useful when using the github PRs ui. |
"any code changes to /src/main that would affect running applications" I don't think anybody runs v2 yet, nor is it released officially.
They aren't, but due to the different approach reactive-streams requires, one would need to learn to review the new style of code. That requires code to learn from and we are in a deadlock situation.
Many v1 tests depend on the existence of multiple operators and the dependency graph is complicated. If I implement one operator then port its unit test, that pulls in 3 other operators which have unit tests requiring 3 subsequent operators; 6 degrees of separation in action. By doing operators first, I concentrated on the logic, minimized the potential interference from others' PRs and posted operators in batched PRs so they don't get too fragmented but can be commented on more easily. @davidmoten Sure! |
I'm not okay with this type of change. The point of the reviews is quality assurance, not speed. Version 1.x is mission critical for many companies. Just because one person can move faster than the group does not mean we reduce the threshold of review. We have chosen to adopt this practice for a reason.
I currently consider v2 in a pre-alpha, sprint phase and am okay with @akarnokd doing what he's been doing to bootstrap the effort. It is effectively a massive refactor/port at this point. Once there is unit test coverage, and general functionality comparable with v1, we will then flip into a mode like v1 where we require reviews of all PRs going forward. We are probably a few weeks from that still. The initial review of the ported code base will take several months for people to do, but I felt it would be best if there was functioning code that could be run as a snapshot, and reviewed as a codebase, not via dozens of PRs that are mostly all boilerplate refactoring. Once v2 feels like it is more-or-less working, I will do a 2.0.0-DP1 release to Maven Central, and we will iterate on DP# releases as long as it takes for us to all agree with the public API, performance and quality. That entire process will go through PRs requiring review. |
|
If v2 PRs are to be exempt, I just want it to be an explicit decision instead of trying to make up reasons about why they magically don't fall under the normal policy. In general, I'm still fundamentally opposed to the approach, for the record.
Well this is patently false and the fact that you're trying to argue semantics of policy is truly worrying. Unless you intend v2 to never be used, then every piece of code you write actually does affect applications. And if you think it doesn't, then every PR falls into that argument and can skip their review under the same logic. After all, no one is running HEAD, and even if they are, it doesn't affect their applications until they deploy so there's no reason to review synchronously.
This is a fallacy. Not only are PRs one of the best places to learn these semantics, but a vast majority of code review (including things for v1) are trivial, normal things that apply to nearly every project. The idea that somehow no one else can understand what's happening is ridiculous. |
|
@akarnokd has finished the sprint to port 1.x to 2.x. That branch is now following the PR review model and calm enough for everyone to begin reviewing and playing with. I'm going to take time to start working with it and let it settle for a while before attempting a 2.0.0-DP1 release to Maven Central. Until then, it is available as a snapshot and I suggest people start digging in and filing issues or PRs with suggestions, issues, alternatives, etc. To be clear, virtually all design decisions about 2.x is still up for grabs if there are strong, objective reasons for change. There are still large design decisions to make, such as being debates in #2787 |
Thus far there have been so few committers that we haven't had a formal policy, mostly just an arrangement between @akarnokd and myself. As per #3145 we are seeking to increase the number of people involved in reviewing pull requests and thus will end up with more people merging changes.
I don't want to over police this, but I do think we should formalize to having at least a 👍 from another committer before any pull request is merged that affects code in /src/main.
Exceptions that do not require a 👍 would be:
Things that do require a 👍 would be:
Thoughts? Agreement? Disagreement?
/cc @ReactiveX/rxjava-committers
The text was updated successfully, but these errors were encountered: