Policy for PR blocking?

[Qard]

I think we should clarify and document how we feel about PRs blocking each other. In particular, I'd like to draw attention to #893, which seems to have been blocked on #1650, despite not having any strict dependency on it. While #893 is major and #1650 is not, I think #893 could have easily landed in 2.0.0 but it did not. Now, with 3.0.0 approaching and @petkaantonov not having the time for #1650, I think it makes sense to add #893 to 3.0.0.

This all makes me think, what if we had a shifting priority window? Normally minors take precedence over majors, but when we near the major release window, we should switch to prioritizing getting major changes merged, because if they miss the release they have a somewhat longer wait until the next chance to merge. During that window, minors probably shouldn't be allowed to block majors unless there is something specifically lacking for the major to be deemed "complete" in isolation.

Perhaps we should codify some specific policy for these situations? With more and more contributions coming in, there's more and more chances for PRs to step on each others toes a bit.

[jasnell]

Ok, let's generalize this a bit. We have two PRs (A) and (B), that each touch the same code. Let's say that (A) is semver-major while (B) is semver-minor.

1. If (A) depends on (B), then (B) must land first. By "depends on" I mean that (A) cannot be viewed as being "complete" without (B).
2. However, if (A) does not directly depend on (B), then (A) should be able to land without being blocked by (B), even if (B) causes changes to the same code touched by (A). Note, however, that after landing (A), (B) could end up changing from semver-minor to semver-major, depending on the changes in (A). In such cases, landing (B) first should take precedence.

In the case of #893 and #1650, we should go ahead and get #893 landed. The improvements in #1650 are great to have, but are a lower priority.

[Qard]

That's basically my feeling on the matter. I'm wondering if we need to decide on a specific protocol for these sort of situations though. I think the main reason #893 got blocked is because Petka suggested #1650 get in first, but I'm not sure that was a valid reason to block it. It might be nice to have a specifically documented policy we can point to when those sort of comments come up in the future.

[jasnell]

@nodejs/tsc : do any of you have specific thoughts on this?

[rvagg]

My only thought is whether this needs to be formalised rather than leaving it to interested parties to champion their code and argue for the way forward, as we do with almost all other changes. In this specific case, I'd like to see @yorkie or yourself @Qard, since you seem concerned, to start arguing for an alternative path.

I'm not going to block formalisation, I'm just not sure how much you can generalise from this case and would prefer to see our standard "if you want to make a change, you have to convince others to get it in or bad luck" work in this case.

[trevnorris]

Also need to consider the case where developer (X) has patch (A) and developer (Y) has patch (B).

Say (B) is blocked by (A) finishing, but dev (X) for some reason can't finish it up. Does someone finish it for them? If the majority of the code was dev (X)'s are they author?

Just a couple scenarios to think about.

[rvagg]

We've had a few of those already (I've been involved in a couple), mostly it works by simply taking someone's commits into your own PR and extending them with your own; leaving theirs intact in the history.

[chrisdickinson]

Err, #893 isn't solely blocking on another PR. It also stands to break a lot of code (for many of the same reasons the other URL change broke a lot of code.) Specifically, since folks tend to parse strings into Url objects, then change a few properties and re-format them with url.format, adding path as a parent key controlling pathname breaks downstream code.

[chrisdickinson]

IOW: we absolutely should not merge #893 as it stands, since we haven't gone through a messaging cycle about "Hey, ecosystem! Your url formatting code will break a few major versions from now, here's how you fix it!"

[jasnell]

Yup. The dev-policy requires at least one major release cycle...
Preferably, the new function could be put behind a flag and the current
behavior marked as deprecated in the interim. Then after at least one more
major the new behavior can become the default.
On Jun 29, 2015 5:14 PM, "Chris Dickinson" [email protected] wrote:

IOW: we absolutely should not merge #893
#893 as it stands, since we haven't
gone through a messaging cycle about "Hey, ecosystem! Your url formatting
code will break a few major versions from now, here's how you fix it!"

—
Reply to this email directly or view it on GitHub
#2078 (comment).

[Qard]

@rvagg While I would normally agree with not introducing too much process, that suggestion sounds dangerously close to "loudest voice wins" to me. I'm sure we don't want PRs to devolve into arguments about which has more immediate merit. I think a formal policy could help to prevent that.

That having been said, I made this issue more to solicit feedback about if we think it's something we should formalize, not as a demand that we do so.

[yorkie]

I was suspended by #1650 just is because the 2 PRs have introduced the getter and setter into the url object, but #1650 seems more complete, so I'm absolutely agreed on finishing #1650 at first, so that I can resolve the @chrisdickinson's concern later :)

[Fishrock123]

Transcript from the last TSC meeting:

https://docs.google.com/document/d/1TN3Ks0fC4ciY3jeS0VxXxXRkT_dq8-tfs-bWQqZGRoE/edit

• Steven Belanger tagged this issue: can we come up with a policy for dealing with PRs that block each other?
• James clarified that this specific issue was about the url parsing changes, there were a couple of PRs that blocked each other
• Chris: the PRs here are a “hot stove” situation where they all break url functionality so they are not trivial
• Chris listed some of the blocking issues that are mainly held up because people are too busy so we probably need a way to communicate status to the community
• James: would like to table this issue until it really becomes a problem
• Bert: we could introduce some workflows - e.g. if you introduce an issue that is blocked on something else then you could mention that it’s blocked
• Jeremiah: more relevant to communicate when things are in the pipeline and provide feedback for timelines so people are reasonably blocked but can be unblocked if there is no activity
• Rod: doesn’t feel like something that needs a policy, perhaps it’s an open source 101 skill
• Mikeal: the problem is that often people who are blocking each other don’t have context so need shepherding by someone with context.
• Rod: doesn’t seem like there’s actionable items here, come back when there’s something more concrete to discuss
• Bert: the workers PR has stalled because of Petka’s lack of time, anything that it’s blocking should just move forward, url changes are unlikely to get in as they are now because they are so breaking

I think the general consensus is that this is probably a misunderstanding and ends up having to be handled case-by-case anyways.

@Qard does this suffice, or is there more we should be doing?

[Qard]

I think we can put this issue aside for now. As Rod said, there's not much immediately actionable here.

I created this thinking it could become more of an issue in the future, as we have more and more PRs coming in. In this case, it was minor. If it comes up again, we can always revisit the issue.