You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Decide and document team practices around PR merging and repository status #377
In our development conventions we don't currently have anything about process for how to coordinate around pull requests. We should decide on some basic practices. I think these should be our guiding principles:
make sure that our codebase is top quality
make sure we build tools with many perspectives in mind (both from a developer and a user perspective)
design for a globally-distributed team that is all awake at different times
ensure we provide space for discussion, if needed
don't make this process so heavy-duty that it is cumbersome and significantly slows down development without obvious benefit
Some questions to answer
Here are a few questions that we should have answers for. I'll also provide a suggested answer we can follow to spark discussion.
Are there some repositories that we need to treat more strictly than other repositories?
How long should a PR be open before it can be merged? Is there a minimum time?
Is it OK to self-merge a PR if nobody has commented after some time?
What happens if there's a disagreement about the Pull Request? How do we resolve it?
Distinguish between stable repositories and work in progress repositories. The way to distinguish these two states is whether a minor release has been made (i.e., 0.1.x). While a repository is WIP, we generally don't adhere to a strict set of coordination conventions (though, team members should communicate with one another what they are up to).
Distinguish between significant and non-significant changes. Significant is hard to determine, so use your best judgment. A good guideline is "anything that makes a user-facing change, that changes how things work under-the-hood in a non-obvious way, or that touches a significant chunk of the codebase.
And follow this general guideline:
For stable repositories with a significant PR, leave it open for at least 48 hours since the last significant commit before merging, unless another team member has approved the PR (or merges it themselves). You're also highly encouraged to ping people or teams that you think are particularly relevant for the conversation.
For non-stable repositories or non-significant PRs, people may self-merge without waiting for 48 hours, though again, use your best judgment about whether other team members may wish to take a look first.
And when making decisions, roughly follow these two principles
rough consensus. Essentially, this means only saying 👎 to a proposal if you think there is a major problem with it, not just if you think that it could be done better or differently.
lazy consensus. Meaning that we assume that a lack of negative input on a proposal/PR/etc is indication of consensus, even if people don't explicitly say 👍. Note that this will only work if we follow the principles above about leaving open PRs and proposals with enough time to let others take a look.
If we have no consensus and people are strongly on opposite sides of a discussion, we can take a majority vote from the people with commit rights. The majority vote gets adopted.
I think these would be great to formalise and add to the executablebooks.org site. I have been referencing that site more and more.
It may also be worth flagging core and non-core packages. Here I am thinking about the core packages being:
myst-parser
myst-nb
jupyter-book
sphinx-external-toc
while others such as:
sphinx-thebe
should be feature adding with more independence (in general). I would suggest any core package should not be merged without review while others could have updates made by appropriate maintainers. What do you think about that / or a step to far?
@choldgraf I think we could add a COMMUNITY.md guide that looks at how we want to grow the community, merge PR's, ec. and what practices we adopt as an organisation to aid that aim. It would be a good centralised place to add items like this.
Another thought here since we just had a round of PRs from the pre-commit.ci service. I suggest that we also adopt a practice of:
wherever we use this or similar services, it is OK for any maintainer to merge auto-PRs to update dependencies, provided that our test suites pass and the changes do not affect user-facing behavior.
Update: feedback about making the provenance of decision-making more clear
We recently got some great feedback that the decision-making process for many of these projects is opaque and hard for others to follow and participate in. There are many cases where we are self-merging things, or merging without a clear connection to open discussion and decision-making. This makes it hard for other people to participate in our processes, and makes it more likely that we will over-fit our codebase to the ideas of individual people.
I propose that in addition to our practices for code review, we also consider the following policies:
tl;dr: Every significant pull request must close an explanatory issue that has been open long enough for others to provide feedback.
Issues that propose significant changes must have at least the following before a PR merges them:
Enough context to understand why something is being proposed. E.g., a rationale for why the change should happen (e.g. the problem it solves), and enough context so that an external person can understand what is being proposed.
A clear proposal for the user-facing change to make. Not a proposal about how to implement something, but what effect it should have on the end-result. This is the "what", not the "how".
When a pull request is created, it should reference and/or close the issue where discussion and proposal has been made. There can obviously be iteration since implementing something often changes the end-user design, and we should treat this as a "good faith" effort rather than something to strictly enforce. The rule of thumb should be "if you think this is significant, give ample time and invite feedback from others".
Activity
choldgraf commentedon May 28, 2021
Here's one proposal:
Make these two distinctions:
0.1.x). While a repository is WIP, we generally don't adhere to a strict set of coordination conventions (though, team members should communicate with one another what they are up to).And follow this general guideline:
And when making decisions, roughly follow these two principles
mmcky commentedon Aug 31, 2021
@choldgraf thanks for starting this discussion.
I think these would be great to formalise and add to the
executablebooks.orgsite. I have been referencing that site more and more.It may also be worth flagging
coreandnon-corepackages. Here I am thinking about the core packages being:myst-parsermyst-nbjupyter-booksphinx-external-tocwhile others such as:
sphinx-thebeshould be feature adding with more independence (in general). I would suggest any
corepackage should not be merged without review whileotherscould have updates made by appropriate maintainers. What do you think about that / or a step to far?mmcky commentedon Sep 8, 2021
@choldgraf I think we could add a
COMMUNITY.mdguide that looks at how we want to grow the community, merge PR's, ec. and what practices we adopt as an organisation to aid that aim. It would be a good centralised place to add items like this.choldgraf commentedon Sep 28, 2021
Another thought here since we just had a round of PRs from the pre-commit.ci service. I suggest that we also adopt a practice of:
choldgraf commentedon Oct 6, 2022
Update: feedback about making the provenance of decision-making more clear
We recently got some great feedback that the decision-making process for many of these projects is opaque and hard for others to follow and participate in. There are many cases where we are self-merging things, or merging without a clear connection to open discussion and decision-making. This makes it hard for other people to participate in our processes, and makes it more likely that we will over-fit our codebase to the ideas of individual people.
I propose that in addition to our practices for code review, we also consider the following policies:
tl;dr: Every significant pull request must close an explanatory issue that has been open long enough for others to provide feedback.