-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Create Reviewer Guidelines #16789
Conversation
NB, check failure is unrelated; seems the dep check on macOS requires Spyder Kernels 2.x (which is compatible with the current Spyder 5), but 3.x is installed (presumably for the future Spyder 6). I'm guessing the former is in error, but I'll leave that determination to @ccordoba12 or someone more actively involved in this area. |
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.
Hey, thanks for submitting this @isabela-pf !
In order to be able to feasibly review this, it would be very helpful to make a few critical changes that would greatly ease following reviews. Most importantly, can we convert this to one-line-per-sentence? This hugely improves the usability of editing, review comments, Github suggestions and git/hub diffs and is the standard in many/most of our other repos, and for the newer meta-files here; GitHub suggestions in particular really don't work well with hard breaks. Also, two breaks before L2 section headers helps them stand out more and follows existing practice in the other docs on this repo, and there's a bunch of trailing whitespace (due to the injected hard breaks).
Since it was pretty straightforward to do with some regex-fu, here is a Gist with just those changes applied; if you're okay with that, feel free to amend your commit with that, or I can do so if you prefer.
As to my other comments that I'll be able to make once that's done:
They basically fall into three categories:
- Clarifying that the section currently labelled "docs" applies to more specifically to reviewing prose content more generally, including the website/blog, API docs, and similar, rather than to a particular named repo, and equally the code review guidelines apply to the code contributed to a Spyder repo (including the docs, which contains a fair amount of code). I meant to bring this up earlier, this makes a lot more sense than having different rules on a per-repo basis, since the rules are generally appropriate to the type of content rather than whatever particular repository it happens to be in, and repos generally contain some of each (and a PR may touch both). I'll start this off with a comment where we can discuss this further before proposing the specific changes.
- Sharing some concerns introduced with the late-breaking changes immediately prior to this PR being opened that I wasn't able to adequately review
- Bringing up a few other issues and points for discussion that I've noticed upon further reflection and review, or are more specific to the final format
Thanks again, and looking forward to hearing others' feedback as well.
I'm multitasking so I think I didn't do that amend quite right (that's not the commit messages I intended…), but I believe I've made the change to sentence-per-line. Thanks for providing the Gist! |
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.
Hey @isabela-pf , thanks again for all your hard work on this! Here's my primary review pass with a number of relatively small, uncontroversial suggestions, mainly focused on fixing some links and stray words, avoiding repetition, catching some mistakes/issues in my own writing and changes, and tweaking phrasing a handful of places for clarity and accurately. I'll have a followup review shortly with a few more substantive questions for us to discuss. 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.
As a followup to the previous detail-oriented review, this one is focused specifically on a few bigger issues/proposed additions that I wanted to offer for discussion before suggesting specific changes. While in many cases they didn't necessarily relate to one specific line, I've made them as comments so we can organize our discussion into more manageable threads.
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.
Just a few suggestions you applied that got lost in the merge, a handful of small tweaks to headings, and a couple of final suggested changes to new and existing text.
As a side note, given the accidental no-message commit, spurious merge and reversion of interstitial changes, this PR might be a great chance to try out our new prototype policy and go with a squash merge, though given the otherwise well written and thought out commits and importance of the history here, I wouldn't want to loose that either and would be happy to help guide @isabela-pf with a interactive rebase to fix these issues, if she prefers. |
@CAM-Gerlach I'm fine with a squash merge, especially as a test. Thanks for asking. |
I think I've either made changes based on all the above reviews or left comments. Let me know if I've missed any. Overall, though, I'd like to note a few things.
|
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.
Just two small changes that got applied previously but lost in the merge conflict (that I commented about earlier, but forgot to mark unresovlved, sorry), and a suggestion to hopefully address @dalthviz 's comment.
Other than the concerns over mandatory explicit [required]
tags on all review comment, and deciding on a path forward for ensuring consensus among and support among the other members of the project, I'm more or less ✔️ on this PR as currently committed.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Added the slightly revised [comment]
tag as a suggestion to both sections, and a line to both clarifying that required questions are included under the [requested]
tag.
Okay, everyone, we have now have a full draft that can be reviewed and voted upon. Since I didn't get feedback on the prior comment about voting, I'll be pulling from that until further notice. Please vote using the following emoji reactions on this comment. If no vote has a simple majority (>=50%) among the non-abstaining voting developers after two weeks (or earlier, if a absolute (>50%) majority of all devs who have not actively abstained is reached), then we won’t merge this PR. The ideas are still open and available for revision, but they would have to become a new PR to be merged. Voting will be open from the time of posting this comment until a week later. That means voting will close and a decision will be made at end-of-day Tuesday, February 8 Anywhere on Earth. I've also been asked to ping @spyder-ide/core-developers to join voting. Thanks in advance for your time! |
@ccordoba12 @isabela-pf @CAM-Gerlach, the macOS App bundle always fails on merging into master because of a spyder-kernels version compliance error. Do we just ignore that and merge anyway? Do we want to fix that somehow? |
@mrclary I actually just spoke with @ccordoba12 about that at today's UX meeting, and he said he will fix that before we merge this, and I'd help @isabela-pf rebase this PR accordingly. |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
f0b74cd
to
7ecb46d
Compare
After a tedious slog, I resolved the issue with the merge conflict resolution via merge that prevented the branch from being cleanly rebased on I marked the PR as a draft to remind @ccordoba12 , or whomever merges it, to make absolutely sure to squash-merge instead of normal-merge to avoid polluting |
Contributors should be explicitly discourage from merging their own PRs. While it generally not appropriate to merge one's own PRs, this does happen, and appropriately so in many circumstances. So it does not seem correct to have a strict rule against it, but it should be explicitly discouraged. Where possible, excepting circumstances should be articulated. This PR provides guidance pertaining to how PR reviews should be conducted, but it may be useful to also provide some guidance pertaining to initiating and closing review requests.
|
It would definitely be helpful to have an "Experts Index", an overview of high-level areas of expertise, e.g. as a table in the contributing guide, so long as it is kept up reasonable to date. It needn't be too detailed or complicated, just a basic list of us core devs and our areas of interest (e.g. macOS packaging, etc. for you, docs, website, etc for me, LSP and Qt for Edgar, etc.). It could then be simply linked to here as general guidance for PR authors and triagers. In addition, while not perfect, we could use As a backstop, we could also have someone knowledgeable responsible for spending a few moments triaging new PRs, i.e. directing them to the appropriate reviewers.
Whether a requested reviewer's review is required or optional really depends on the PR and the repo involved, and unfortunately GitHub doesn't provide a straightforward, unambiguous way of signaling this. CODEOWNERS + branch protection does allow a limited form of this, where required reviewers are automatically assigned based on the files changed, but I'm not aware of a way to manually mark reviewers as required or optional for situations where this isn't sufficient. For example, on the QtPy repo, typically @dalthviz ' review is required every PR, @ccordoba12 also reviewers more important ones, I am generally responsible for reviewing those related to CI/linting/etc,, and generally at least two out of the three of us review every PR before merging, with review requests typically reflecting that. This was basically the same as it was the docs, with me and Juanita signing off on every non-trivial PR, and Carlos as well if it was substantial enough (which was developed into a formal policy at @ccordoba12 's initiative, after a similar though a bit more serious set of instances where a certain Spyder lead maintainer 😉 merged their own PRs without both the others reviewing, and drama ensued). Generally speaking, if someone was requested for review, it was understood that their review was required. On the other hand, on e.g. docrepr, I cast a wider net tagging Slyvain and several of his developers to get a faster review from whoever's available, with any one of them sufficient for approving and merging, and for small PRs on other repos, I sometimes find myself doing this as well to get a second look before merging soemthing. On yet other projects, like on the PEPs repo, some reviewers are required (e.g. the PEP author and/or their core dev sponsor, or the maintainer of the given infra component, which usually indicated by codeowners, but not possible in cases where the author is not a Python org member), while some are optional and only review from one of them is required (e.g. the PEP editors tagged to provide proofreading and feedback; there is a mechanism to tag a team that only requires one review from someone on it, but often I was to target specific members based on the PR content and their previous involvement in it). So the answer is, its complicated 😆
+:100:
I used to be strongly in favor of this (from past experience in Spyder-Docs), but now having a wider experience with different projects/repos and their needs, I think this is a sensible default for the Spyder org but can be overriden by per-repo policy. Different repos have different needs and according expectations when it comes to whether reviewers are AND and OR, and its easy for someone to work around (or abuse) this by simply removing review requests of people who haven't reviewed.
We could use the Assigned field for this; we sometimes use it to indicate when a PR is being worked on by someone other than the original author, but otherwise it is mostly redundant. |
Another item we may want to discuss is when to rebase if there are no merge conflicts. If there are merge conflicts, it makes sense to rebase and resolve any conflicts. This also ensures that the CI tests pass under the latest master and proposed merge changes. However, if a PR parent commit is more than a few inconsequential commits behind the latest 5.x commit, even if there are no merge conflicts, the unit tests will not have been run against those later commits. Correct me if I'm wrong, but I think the unit tests are run on the last branch commit, not the resulting merge commit, so rebase is required in order to get accurate unit tests. However, any rebase will require reviewers to re-approve, so some balance must be reached between merge delay and respect for reviewers' time on the one hand and accurate CI tests on the other hand. Guidance on this may have to be at the discretion of the reviewer(s). |
@CAM-Gerlach, thanks for your comments. It looks like we agree.
👍 It may be easier to implement this in the short term and, if needed in the future, escalate to your other ideas such as a triager or CODEOWNERS.
Exactly. I think we both understand it to be a process, not a rule. We should just pick something. Even if it is revised later, being explicit in the guidelines is better than being implicit.
👍 And if we find it doesn't work well, we can revise it.
👍 I think that's a great idea. |
This is correct.
There is a branch protection setting to require the branch to be up to date before merging, so this doesn't happen. I've toyed with the idea of enabling it for various repos, and do it as an informal practice myself, but so far have mostly held off from enforcing it in branch protect, because on the larger and more complex repos (like the main Spyder one) where it has the most value, the test suite also tends to takes much longer to run (I've noticed runs of an hour or more here are common) and have a higher probability of random failures, meaning each PR would need to be rebased and wait its turn to be merged one by run after rebasing and running the test suite, which could greatly decrease development velocity. At least personally, I've found these sorts of failures to be pretty rare, and they are still caught by the CIs on push after merging and before any release is made. This also adds extra burden to contributors, and I particularly worry that it will result in them merging rather than rebasing on master, as they have an unfortunate tendency to do, which makes the commit history, squashing and rebasing much harder to follow and work with. So I'd suggest at least not hard-enforcing this requirement with branch protect, and instead requesting it when needed on a case by case basis for major changes.
It is configurable in the branch protection settings whether new changes dismiss earlier approvals, but yes, this is indeed a tradeoff. |
Just as a side note, there are also some steps after something gets merged that maybe could be worthy to reference here. They are at https://github.com/spyder-ide/spyder/blob/master/MAINTENANCE.md mostly regarding the way to keep branches up to date between each other (like master vs 5.x for example) or when a merge also involves updating the sub-repos |
Yeah, we actually brought that and some related issues up at our meeting yesterday and I was initially in favor of adding them here, while the others leaned toward putting them in a separate document. What we were thinking of was that there are a fair number of meta-files in the repo root now, and a lot of them are common to all our repos, we could move common policies (specifically But since you're the one most involved besides @ccordoba12 in this stuff, we wanted to get your input first on that. Also, there was still some debate over the name; @ccordoba12 and @steff456 preferred some variant of |
|
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 to @isabela-pf for helping us to draft these new guidelines and everyone that participated in the discussion around them.
Description of Changes
This PR adds
REVIEW.md
to the root directory. This document contains the draft Reviewer Guidelines myself and multiple Spyder reviewers would like to add to the project's contributing process.This document has mainly been a collaboration between myself, @juanis2112, @steff456, @CAM-Gerlach, and @ccordoba12. Review is absolutely welcome!
CONTRIBUTING.md
?I'm looking to merge to
master
because the contributing guide lists that this is for breaking changes. This is a breaking community change and not tied to any particular software release. Please let me know if it needs to be merged to a different branch.Issue(s) Resolved
There is no related issue. This document was written in response to a review of Spyder's current Code of Conduct and to help make reviewing pull requests more clear for reviewers and PR authors.
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct:
isabela-pf
Pinging @spyder-ide/core-developers for review per @ccordoba12's request. Thanks!