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" notifications@github.com 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 :)