PR: Create Reviewer Guidelines

[isabela-pf]

Description of Changes

This PR adds REVIEW.mdto 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!

• (n/a) Wrote at least one-line docstrings (for any new functions)
• (n/a) Added unit test(s) covering the changes (if testable)

• (n/a) Included a screenshot or animation (if affecting the UI, see Licecap)

• Link this document to other relevant community engagement resources. Maybe it should be pointed to in 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!

[CAM-Gerlach]

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.

[CAM-Gerlach]

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.

[isabela-pf]

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!

[CAM-Gerlach]

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!

[CAM-Gerlach]

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.

[CAM-Gerlach]

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.

[CAM-Gerlach]

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.

[isabela-pf]

@CAM-Gerlach I'm fine with a squash merge, especially as a test. Thanks for asking.

[isabela-pf]

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.

1. This PR is a proposal, and an experiment at that. It doesn’t have to be accepted. I’m not regularly a Spyder reviewer, so I also want to say that these changes will impact me less than many of the people commenting on this PR. Based on some of the comment threads above, it doesn’t sounds like reviewers actually want this. If that’s the case, then this PR shouldn’t be merged. I think one of the worst things we can do is break promises we make to the community, so we shouldn’t have guidelines we are going to ignore.
2. Because this PR links itself to the Code of Conduct, I know it needs ample review. Based on how this has been going so far, I’m not going to be able to keep up with the requested changes in the limited time I have for this. This PR is based on a document that’s been drafted and reviewed by multiple people for a month prior to the creation of the PR (it was requested that I not start with a public draft PR and I honored that). It may be best if I close this PR and someone else opens a new one (with the same content, if wanted) to steward these changes if we expect more substantive review to this document to come.

[CAM-Gerlach]

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.

[CAM-Gerlach]

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.

[isabela-pf]

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.
👎 Do not merge
👍 Merge and have a trial period for the guidelines (I propose three months), time for edits based on what we learn, then vote again on whether or not to merge this PR. This would separate it from the Code of Conduct for the trial, to be clear (and that will be noted at the top of the document with a final commit).
👀 Abstain from voting

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!

[mrclary]

@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?

[CAM-Gerlach]

@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.

[CAM-Gerlach]

After a tedious slog, I resolved the issue with the merge conflict resolution via merge that prevented the branch from being cleanly rebased on master. I make sure there was no delta with the final product (i.e. git diff was empty).

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 master's commit history, since I didn't individually fixup, squash or re-write the messages of individual commits (except for the merge-conflict one), even those that were originally made by mistake.

[mrclary]

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.

1. It may be a good idea to have a list of general areas of Spyder with appropriate reviewers listed for each. One issue facing contributors, particularly new or inexperienced ones, is choosing appropriate reviewer(s). While Github suggests possible reviewers, This will be different than just a list of developers that have contributed significantly to a particular source code file, but topical based, such as "macOS installer" or "plugin architecture", or whatever. A PR may span many UX elements and source code files, making it difficult to determine which reviewer(s) are best suited. A list of appropriate developers will help contributors be more confident in their selection. Choosing appropriate reviewers will also tend to elicit more prompt responses, as the PR deals with areas of expertise/interest to the reviewer.
2. We should advise contributors to request as few reviewers as possible; when in doubt, select only one reviewer. Another issue facing contributors is choosing how many reviewers to request. It is desirable to have a broad consensus on proposed PRs as well as to have many sets of eyes to catch potential mistakes, but having too many reviewers may unnecessarily delay merging because there is an implicit expectation that all reviewers should approve the PR before merging should occur. Everyone is busy, so the more reviewers required for approval the greater the likelihood of delay. Of course, any reviewer may add reviewers to the PR if they think it appropriate, or request the contributor to do so.
3. Reviewers should respond as soon as possible to a request to review a PR by acknowledging the request or recusing and recommending/assigning another reviewer.
4. We should require all reviewers to approve the PR before merging. If recommendations 2 & 3 are followed then any superfluous reviewers will have been removed and it becomes reasonable to expect approval from all remaining reviewers.
5. We should designating one of the reviewers to execute the merge request.

[CAM-Gerlach]

It may be a good idea to have a list of general areas of Spyder with appropriate reviewers listed for each.

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 CODEOWNERS (which has been improved a lot very recently) to help automate that. It works well for us on the Python PEPs repo, and I believe on the core CPython repo as well.

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.

We should advise contributors to request as few reviewers as possible; when in doubt, select only one reviewer.

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 😆

Reviewers should respond as soon as possible to a request to review a PR by acknowledging the request or recusing and recommending/assigning another reviewer.

+:100:

We should require all reviewers to approve the PR before merging. If recommendations 2 & 3 are followed then any superfluous reviewers will have been removed and it becomes reasonable to expect approval from all remaining reviewers.

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 should designating one of the reviewers to execute the merge request.

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.

[mrclary]

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).

[mrclary]

@CAM-Gerlach, thanks for your comments. It looks like we agree.

It would definitely be helpful to have an "Experts Index"

👍 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.

We should advise contributors to request as few reviewers as possible; when in doubt, select only one reviewer.
So the answer is, its complicated 😆

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.

We should require all reviewers to approve the PR before merging. If recommendations 2 & 3 are followed then any superfluous reviewers will have been removed and it becomes reasonable to expect approval from all remaining reviewers.

I think this is a sensible default for the Spyder org

👍 And if we find it doesn't work well, we can revise it.

We should designating one of the reviewers to execute the merge request.

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.

👍 I think that's a great idea.

[CAM-Gerlach]

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.

This is correct.

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.

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.

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.

It is configurable in the branch protection settings whether new changes dismiss earlier approvals, but yes, this is indeed a tradeoff.

[dalthviz]

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

[CAM-Gerlach]

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 CODE_OF_CONDUCT, REVIEW (this document), and possibly guidance on when to merge) as well as possibly copying common/generic portions of our Contributing Guide, Maintainer Guide, Release Guide, etc. to a dedicated org-wide repository, and then link that repo from the various subprojects.

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 governance, while I favored something more like general to encompass the scope of the documents, e.g. common-guidelines, common-policies or similar.

[CAM-Gerlach]

⚠️ ⚠️ ⚠️ IMPORTANT REMINDER to Carlos: Don't forget to squash merge! ⚠️ ⚠️ ⚠️

[ccordoba12]

Thanks to @isabela-pf for helping us to draft these new guidelines and everyone that participated in the discussion around them.

Addressed already