ENH: Clarifying merge and review practices

[choldgraf]

This is a proposal to clarify and make more explicit our policies and guidelines around PR merging and reviewing. See issues below for discussion and (I think) suggested language. I tried to strike a balance between "very specific and rigid" and "so generic that it is hard to use this to make decisions".

• closes Recommend (or require?) PR workflow for contributing to repositories #465
• closes Clarify our merge policies vs. guidelines #594
• closes Guidelines regarding self-merging PRs #466

[consideRatio]

Thanks for your work to clarify this @choldgraf!!!

I'd love to see if there is sufficient agreement for my proposal of:

• We default to merging on approval
• When not merging on approval, we clarify why not and help nudge the PR towards resolving.

It doesn't have to block this PR though!

[choldgraf]

Thanks for the suggestions all - I've made a few edits to this one to address @consideRatio's comments here as well as a few in the issue referenced above!

[manics]

Thanks for working on this.

The only controversial suggestion I see is "each pull request should reference at least one issue", I don't think this is something we've encouraged in the past.

[manics]

each pull request should reference at least one issue

This is a big change. In the past we've actively discouraged people from opening issues alongside PRs as it leads to split discussions.

It's advisable to open an issue before you introduce a time-consuming change, but even then my feeling is that as long as you're aware that you're investing time in writing code that may not be merged that's fine- having code to illustrate a difficult or controversial change often helps to illustrate the issue.

[minrk]

Yeah, I agree. I wouldn't want to imply that we want folks to open an Issue immediately before opening a PR that closes it. PRs are issues, so I find having a discussion in one place for an issue and its solution is often preferable to having one discussion in two places. If I found a problem, I would only open an Issue first if I knew I didn't have time to open a PR instead, and I'd encourage others to do the same. I think that's easier to keep track of for ~everyone unless you are doing specific tracking/reporting based on closed Issues only, which we don't.

If folks want, we could mention the reasons for opening a PR against an already acknowledged issue, and the potential reasons to open an issue first to discuss a change before working on a solution. I mainly don't want anyone at the stage of 'I have my change, time to open a PR' deciding they need to open an issue first for some reason, and I feel like this text is telling folks to they should do that, when the opposite is true.

[choldgraf]

Ah ok - I will remove this then. My goal in this PR is not to propose anything too new.

For what it's worth, I think it's more important to use issues and discussions. I think GitHub is already not very accessible to non-developers, and pull requests are the worst place to make decisions if you want to include non-developers.

I also think that doing all of the work in just a PR (for non-trivial changes) conflates two distinct parts of the process that require input from different stakeholders :

1. Agreeing on the problem to solve, agreeing that it is worth solving, and aligning on a general idea of what the solution looks like (I think both of these should be in issues, a discussion forum, or a JEP)
2. Agreeing on an implementation to the problem and potentially clarifying design considerations that come up during implementation (i think this needs to be PRs)

Obviously I don't think that's a strict process for all changes - if you're fixing an obvious bug, sure just go for it. But for anything non-trivial or user-facing, yes my preference is to always have an issue or forum topic for discussion first.

How about I try to add nuance like "if the change is non-trivial, try to open an issue to discuss first" in the guidelines? I think that is in line with our other practices around when to intentionally slow down the merge process

[manics]

One concern I have is that we're struggling to stay on top of all issues and PRs across our repositories. Long-lasting issues and PRs are frustrating for contributors or users, as is the lack of well defined process in some cases, but that's partially balanced by being an open-source project which engenders some goodwill and understanding of how open-source works.

That goodwill is lost if we ask contributors to follow a certain process but don't follow-up- if I'm interacting with a project with no guidelines it's frustrating but I understand that the voluntary nature of open-source means things can be a bit haphazard sometimes. It's a lot more frustrating to have guidelines which I've followed but which aren't reciprocated due to lack of time or resources.

[choldgraf]

I think this is a good point. I will try and change the wording tomorrow to be more nuanced

[betatim]

Big 👍 to not encouraging people to create issues a few seconds before they create a PR that closes it. For the reasons that Min and Simon articulated. I think a great strength of a PR is that you can have all the discussion and the change right there, basically all the context you need to take action (review, merge, etc) is right there.

On the topic of keeping discussions separate (should we even solve this? vs how to solve this?) I think doing it all in the PR discussion, especially if there is code to look at makes sense. In rare cases, for contentious changes it might make sense to move the discussion to an issue to try and separate it from a particular implementation (and its author). Basically a way to grease the social wheels of making progress with reduced emotional attachment to a particular change. But I think this is rare, so we can do it ad-hoc.

Sometimes the result of a PR might be "this is a problem, but somehow this doesn't feel like the right implementation and we don't know what the right implementation looks like/don't have time to work on it" - in these cases I think it makes sense to open an issue with a short summary of what happened in the PR to "reboot" the conversation and make it easier to get into it when revisiting it later.

[choldgraf]

I've re-worked the language in the latest commit to be softer on this statement, can you focus discussion on that language to suggest edits etc? I'm not going to debate when we should/shouldn't open issues (my opinion is that we should always open issues, but I recognize I'm in the minority on that one and I don't feel the need to convince people here as long as the language in this PR is OK to y'all). I'm happy to follow whatever policy the team wants here

[minrk]

Interesting! I don't view issues and PRs as distinguishable for conversation purposes in terms of participants or goals, other than PRs having concrete changes to discuss, while Issues don't. Forums/Discussions are definitely a mechanism for broadening participation. I've never had the impression that Issues have different visibility/participation than PRs.

Suggesting opening an Issue for discussion prior to PRs seems sensible. Maybe also suggesting soliciting feedback for large/user-facing changes via wider mediums like the forum even after opening a PR is a good idea, too.

[choldgraf]

My thinking on this changed considerably once I started using GitHub as a collaboration platform with people who were not developers. In my experience, when a non-developer is asked to participate in a discussion, but then sees a page that displays a bunch of source code, they are often significantly discouraged to participate because they feel like they're out of their element.

I also find that putting the code in front of everybody's face tends to focus discussions on implementation-centric ideas, which are often very different from design- and UX-level ideas. Pull Requests are inherently a code-centric thing, and much of the activity around deciding "how should our tools change?" does not require understanding our codebase. In fact, many of the people out there with deep expertise in UX and product design are not coders at all! I think it's important for somebody to be able to be heard without requiring them to look at source code.

Ah I can feel myself slipping into a debate about this 😅 as I say I am happy to just stick with the minimal language that will let us move forward, and I'm happy to have deeper discussions about inclusion and accessibility in our team processes in follow-ups.

[minrk]

Those are excellent points, and I don't think I appreciated the presence of code being off-putting for folks otherwise happy with Issues. My main experience has been that Issues are similarly exclusionary to PRs, not that PRs aren't exclusive. So in the goal of soliciting broader participation, I haven't found increasing the use of granular Issues improves things over discussion in PRs to be beneficial, whereas other lower-traffic, non-code-centric places like the forum/projects/discussions/blogs/social media definitely is.

But I agree, we should always be working to improve our processes for inclusion, and that's a bigger, broader topic. Thanks for always being so thoughtful on this!

[manics]

The only exception I can think of to the PR workflow is for making releases.

[choldgraf]

I felt that it was under the "I'm pretty sure we already implicitly follow this" category which is why I put it in. IMO (like everything else) it's not a strict rule (eg not needed for small dogs changes) but a good guideline

[choldgraf]

OK I believe that I've addressed the comments above by removing the language about "must" open issues, and instead providing recommendations for how to be inclusive of others in discussion for more complex proposals.

[betatim]

I'm not super happy with this. Without historical context this makes me think that we will end up with forum threads that should be issues, the problem being that forum threads can't easily be linked from other issues/PRs.

A proposal that I am more happy with, but also not super happy:

Let's add something to the previous item (include others) about shopping your idea around and maybe how to judge when you should do that/where you should do that. Maybe the rule of thumb is "the more people are impacted by your change, the broader you should shop around your proposal"? - but it might be hard to know how many people are impacted by your change. An alternative is "the more fundamental your change..." - but that kinda has the same problem (how do you know?).

With the above addition I would delete the first sentence here ("Almost any change ... forum post).". That way this item stays on the topic of "you should link to discussions and context". Creating an issue/forum post is a way of creating that context, which is different from then linking to it.

[choldgraf]

I'm not quite sure how to thread the needle with the practice of linking to previous discussions. Could somebody propose specific language in the PR review?

For what it's worth, much of my own thoughts re: issues are derived from Simon W's post on "the Perfect Commit". I find his arguments there pretty compelling.

If folks here have found a similarly useful blog post / essay / etc that makes a case for why you shouldn't have separate issues, and instead should put everything into a single pull request, I would certainly be interested in reading it.

[GeorgianaElena]

Thanks for this work @choldgraf!

[manics]

I think the important thing about https://simonwillison.net/2022/Oct/29/the-perfect-commit/ is that there's a description for what the issue should cover (Background, Links to things, Code snippets illustrating potential designs and false-starts, Decisions, Screenshots, Prototypes), and it refers to linking commits to issues.

As far as I can tell linking a single commit to an issue is the same as a pull request with commits, the only difference being that in the former the commit is linked to an issue through the commit message whereas in the latter it's automatically part of the PR.

[choldgraf]

I took another pass at the text here after feedback from you all, and also incorporated the suggestions from @betatim and @GeorgianaElena. I tried to consolidate a little bit per the suggestions above. I also tried to fix a few more places where I was violating "one line break per sentence". What do folks think about it now?

[betatim]

I like it. There is one of my comments left regarding "try and focus on the big picture (first)" when reviewing PRs. It was triggered by what was already written. But it potentially does what I say we shouldn't do: broaden the scope of a PR by commenting on adjacent stuff ;)

With or without that comment I think this can be merged. It is "not worse" (yeah, I try to have a low bar for merges) than what we have at the moment.

[choldgraf]

@betatim I added one more commit to try and capture what you said, I put it at the top of the list because I actually think it is the most important. WDYT?

3064410

[manics]

Looks a lot nicer now!

Not sure where best to mention this in the doc, but it might be worth mentioning breaking changes as a class of PRs requiring more consideration since they can't be easily reversed, even if it's a small code change?

[consideRatio]

@choldgraf wieee amazing work to tie together so many peoples thoughts!!!! ❤️ 🎉 🌻!!

[betatim]

RTD build failed with " fatal: reference is not a tree: 3064410 ". Trying to figure out if I can restart it somehow. However we saw a RTD build fail in a different JH repo for "no reason", so maybe this is due to something on the RTD being broken at the moment.

[betatim]

Merged. The comment about backwards incompatible changes is a good one, in the interest of keeping people moving along, let's put it in a new PR.

[choldgraf]

Yay! Thanks all for the helpful feedback :-)