-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disambiguate and document the closure process #7516
Conversation
Great improvement! Since this is the only file in ./docs/, WDYT of putting it directly at the root folder of this repository? Maybe also add a link to this doc in the PR section of CONTRIBUTING? |
Thank you for the review Hervé.
I am glad that these ideas resonate with at least one contributor.
Let me take this as an opportunity to illustrate one of the things I discuss in this PR, which is negotiating scope. I agree that having this file in
For these reasons, I would prefer to keep the scope of this PR as-is (i.e., limited to the closure process), and keep any unrelated changes to the directory structure, formatting, and linking of these documents to a separate PR, which hopefully could be reviewed very easily and integrated very quickly. While I am not personally up for filing that separate PR right this minute, I would welcome you to file it if you are interested, and if not I might get around to it eventually in a few weeks once I process more items on my TODO list. |
I really like the idea of assigning a maintainer to a PR on which he helps getting it eventually merged. This would also help new contributers who to ask for help an their PR. So I think the clearer process can help speed up the process of working on PRs. But I guess approvals, especially for the new assigne should be done by the other maintainers which are "more involved". Besides that, for the tables I would add a title, although I do not find a great one. |
Thank you for the review Stefan.
I am glad that these ideas resonate with another contributor. You have understood the primary purpose of this proposal: to provide clear and explicit expectations and lines of communication between core maintainers and contributors, with the goal of a smoother integration process and therefore increased future engagement.
Yes of course.
Good point. I added "Optimal Outcome" and "Suboptimal Outcome" headings to the tables. |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it seems to be a pretty nice proposal. The simple fact that a PR author can have a single point of contact to help them is pretty valuable.
One fear that I have with that, is to increase the entry barrier cost for non-frequent core maintainers. If I am spending a moment of review during a WE, I don't necessarily want to invest even more time to "babysit" PRs, but I could already put some ready-for-merge labels for example. I understand the desire to ensure the promise are fulfilled related to ready-for-merge => 24h etc.
Imagine the situation where two core maintainers are reviewing a PR, one on Saturday the second on Sunday, with the "current" process the PR can be merged, but with the proposed one, none of them will be able to add the label.
No data point, I imagine it's not that often the case. Hence not a blocker at all, just wanting to see it in effect and to see the impact of that change.
docs/MAINTAINERS.adoc
Outdated
In that case, any core maintainer can retroactively assign the pull request to the core maintainer who closed it for tracking purposes. | ||
If a core maintainer does not regularly close pull requests, | ||
the maintainer should consider whether it is appropriate to remain on the core team | ||
in light of the responsibility to closing pull requests that such membership implies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced that should be part of the documentation. That could add more pressure on the people, leading to less engagement.
Inactive core maintainers will not even read that documentation ;)
Thank you for the review Wadeck.
Indeed. Much like #7218 set clear expectations for submissions (e.g., that authors have the responsibility to test their submissions), this PR sets clear expectations for maintainers (e.g., that maintainers have the responsibility to steer the discussion towards justified action items and to confirm that they have been completed). Setting clear expectations reduces friction and increases alignment towards a common goal: the closure of the pull request, ideally via acceptance rather than rejection! This does not mean that we need to have a legalistic or rigid mentality. But think about the discomfort a newcomer might face not knowing what the expectations are on both sides. Having some set of common expectations takes away some of that discomfort.
Yes, requiring that merges be done by the same maintainer who applied the
The tradeoff here is best understood in terms of the correctness vs performance duality. The old system is capable of delivering the desired result in the shortest amount of time (i.e., its best-case performance is optimal), but it sometimes sends the wrong signal to authors (i.e., it sometimes lacks correctness). The new system always sends the correct signal to authors (i.e., it always has correctness) but occasionally loses a small degree of parallelism (i.e., its best-case performance is suboptimal). Recall that my two explicit high-level goals were (1) to deliver value to end users and (2) to encourage new and existing PR authors alike. Optimizing globally for these goals motivates the tradeoff I made in favor of correctness over performance.
As alluded to in the Motivation section, the current low barrier for infrequent core maintainers comes with its own disadvantages for PR authors. PR authors tend to at least hope (if not assume) that individuals with maintainer status (indicated e.g. by the green checkmark) will merge their change in a timely fashion. People coming from the corporate world (where individual contributors are typically expected to do peer reviews) are more likely to have this expectation. Infrequent core maintainers are less likely to meet this expectation, sometimes leading to confusion and frustration for PR authors. As much of a chore as it can sometimes be, shepherding PRs to completion (I prefer this phrase over "babysitting") is a key part of fostering a vibrant FOSS community. I believe it is critical for our community's future. As with the previous example, this is a matter of tradeoffs. Recall that my two explicit high-level goals were (1) to deliver value to end users and (2) to encourage new and existing PR authors alike. Optimizing globally for these goals motivates the tradeoff I made in favor of PR authors and frequent core maintainers and against infrequent core maintainers. Note that nobody is being asked to do any more (or less) than what they are doing today. Self-assignment is strictly voluntary. Nobody will force assignments upon others. The existing contributions of current maintainers are highly appreciated. Nobody will be asked to "step up or step down." But yes this proposal does implicitly incentivize existing active core maintainers to remain active and existing inactive core maintainers to become more active for the benefit of end users and PR authors. I am intentionally seeking to incentivize a virtuous circle, and I believe it is in the long-term best interests of our community. Thinking more deeply about your point, I suspect your point may be more about the negative phrasing of my sentence rather than my genuine desire to incentivize a virtuous circle. Perhaps we could address your point by inverting the phrasing of the sentence in the positive direction: "In light of the responsibility to close pull requests implied by membership on the core team, all core maintainers are strongly encouraged to regularly close pull requests." What do you think?
I assume you are specifically referring to the sentence "if the assignee cannot provide such confirmation in a timely fashion, the assignee should consider whether it is appropriate to remain assigned to the pull request in light of the commitment to the contributor that such an assignment implies."
It does occur from time to time. In one case the PR author asked if it would "make more sense to give this review to another member of the core team" (their words, although my proposal was written first), which supports my notion that PR authors want an explicit point of contact. As with the previous example, I suspect your point is more about the negative phrasing of my sentence rather than my genuine desire to improve PR review outcomes. Perhaps we could address your point by inverting the phrasing of the sentence in the positive direction: "If the assignee cannot provide such confirmation in a timely fashion, the assignee should consider unassigning themselves from the pull request to allow another core maintainer to pick it up". What do you think?
The Zen of Python says that "explicit is better than implicit" and that "in the face of ambiguity, refuse the temptation to guess." Along these lines, the section about voting goes along with the two main themes of this proposal: decreasing ambiguity and providing explicit reasoning as much as possible. This elevates the level of discourse by focusing on objective arguments over subjective opinions, and the Apache project's implementation of these concepts has been particularly useful in past discussions. To some degree I have already been employing some of these practices in my own reviews, so I did not think it would be particularly controversial. If it is, it could be split out into a separate proposal as you describe; if it is not, I think it jives with the current proposal nicely. I would like to hear what others think before going in one direction or another. |
Note that #7516 (review) was posted about a month ago and that I addressed the review feedback within four (4) hours in #7516 (comment). A month later, #7516 (comment) remains unacknowledged. When an author's response to review feedback remains unacknowledged for an extended period of time, this sends the implicit message to the author that their contribution was not valuable. This, in turn, decreases the likelihood that the author will contribute again. The "Speed of Code Reviews" section in Google's Engineering Practices Documentation describes what happens in situations like this:
For this reason, this proposal documents the expectation that maintainers reply to authors in a timely fashion. |
Yes a lot better 👍
In this case it's not the wording but more the inaction that I think is problematic. If an assignee is not active during a period of time, it's unlikely they will come back just to unassign them. Yes it's a quick action but it's more about the "mental load" implied by this. (your comment was created in the meantime)
Christmas ;) |
Great! I have applied the new phrasing.
Good point; I have applied your suggestion.
This response is neither logically cogent (because of the lack of an argument) nor grammatically correct (because of the lack of a verb). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, specially the "actionable feedback from the reviewer" part.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable improvement overall and worth a try IMO. Motivation is a bit unclear as problems being solved are mentioned, but not described further.
The degree of detail and legal-like language are perhaps not ideal ways to motivate people to participate in this process, but I guess we'll see how this goes.
Similarly, many of the responsibilities of the assignee are currently shared responsibilities of the maintainers group. While explicitly stating them seems helpful in creating a common understanding, a potential unintended side effect might be that being not an assignee will be seen as having no responsibility at all, even if there's prior involvement in a PR.
Co-authored-by: Daniel Beck <[email protected]>
Thank you for the review, Daniel!
I tried to strike a balance between not being descriptive enough and being too descriptive by describing the general problems I have observed without needlessly drawing attention to particular examples. I want to focus on the future and not dwell on the past.
I would have preferred to discuss topics like this in a less formal venue, but we currently do not have e.g. a regular meeting for core maintainers.
Indeed, and I consider this a problem because they are not shared equally.
Today a few pull requests get a lot of attention while many pull requests get zero attention. My hope is that this proposal will spread core maintainer attention more evenly. |
Having something like that would be useful IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not been very active recently, but this PR got my attention. First of all, thanks to @basil for documenting some missing bits of the process, and for basically suggesting a new process. Most of the pull request definitely should be merged as something that improves the process documentation. Some bits de-facto change the process though
Not sure I can even count as a maintainer after a long hiatus (announced to the community), but I definitely vote -1 for any process that introduces a veto right that has no normal discussion and override process.
All feedback up until the present time has been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the amendments @basil ! In the current form, the text looks fine with me and it seems to outline the current process and intentions well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with sincere thanks for your efforts.
I reviewed the full document and found a few items that would benefit from a minor update, but none of those items are being modified in this pull request. I'll submit separate pull requests for those trivial improvements.
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks! |
Over the past few months and especially during Hacktoberfest, I have observed several cases where the contribution process did not go as smoothly as it could have gone. After thinking deeply about several examples, I concluded that the existing process contains ambiguities that, once disambiguated, would make the process go more smoothly in the future. In this pull request I have distilled my thoughts into a concrete process improvement. The motivation, as explained clearly in this PR, is to deliver value to end users and to encourage both new and seasoned contributors alike. In this PR I explain how this goal can be more readily achieved by disambiguating the process, and I document how an improved process would work.
Testing done
Visual inspection of the the rendered AsciiDoc to confirm there were no rendering errors.
Proposed changelog entries
N/A
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).