[Discussion] Improving the PR review process for this project

[cbethell]

I am filing this issue to record some improvements that can be made to the PR review process for this project as discussed in a meeting between @jaclyn-taroni and I.

This issue was prompted based on open PR #246, which is a PR that has been open across multiple sprints and now has 100+ comments, which is not ideal for a 2nd reviewer's experience (who may also have high-level comments affecting the structure of what's in the PR which would not be an ideal for anyone involved).

The proposed solution is to follow a PR review structure like the following:

1. File a draft PR to get high-level review comments from members of the team, 
specifically but not limited to those who may not be so involved in the recent updates/additions
to the content in the project already.

An example of said review would include feedback on any conceptual or structural
aspect of the PR (the author may not update comments/documentation in this PR
but will provide adequate information for reviewers to follow along and understand
what the author is accomplishing/looking to accomplish within the PR).

Note: This draft PR can be closed once the feedback is incorporated and high-level
review is completed.

2. A new PR will then be filed, incorporating the feedback from the draft PR, to get a
more thorough review from two members of the team. This will be the nitty-gritty PR
that will ultimately be merged into master.

Note: The author may use their discretion here to decide whether or not the comments
on the second PR are becoming extensive and a new PR should be opened to address any
further feedback. In the case of this project, the author may then make their case on the PR
as to why it should be merged and a new PR opened, and upon approval from the reviewer, 
get the second (current) PR in a condition to merge and merge it into master.
Then, a subsequent PR can be opened to address the further suggested changes and continue
PR review.

This will hopefully decrease the amount of comments on the PR for an easier review experience, and also address higher-level comments early on in the process for a smoother experience overall.

We want to take a first attempt at this plan for one of the upcoming series of pathway analysis PRs (which will be after wrapping up the ortholog mapping PRs and some other before-testing issues), so feel free to leave comments on this issue below with any suggestions or concerns you may have!

[jashapiro]

This sounds good!

One other thought I had was that it may make sense to explicitly mark sections of text and or code as DRAFT or OUTLINE so that the reviewers can focus on one or a few sections at a time for review. With reference to #246 (just because it is there), this might have taken the form of getting the base code working in a first PR, but leaving the section on collapsing of multimapping for a separate PR. We kind of knew early on that was a section that was going to require discussion, but in retrospect we probably didn't need to delay the rest of the notebook while we mulled it over.

[jashapiro]

Related: we might consider reopening #245 so that early drafts are not linked into the site navigation until they are considered done. If we are breaking up review into smaller chunks, then we would not want people to end up going to "unfinished" pages. This will be more of a concern after we go "live", but we may as well think about it now!

[jaclyn-taroni]

Related: we might consider reopening #245 so that early drafts are not linked into the site navigation until they are considered done. If we are breaking up review into smaller chunks, then we would not want people to end up going to "unfinished" pages. This will be more of a concern after we go "live", but we may as well think about it now!

Once this is "live" - would moving to a model where there is a staging branch (#228 (comment)) be better than just not linking in the navbar for development? I feel that "lowering the stakes" by not having something up on GitHub pages until it's ready for release might make it easier to iterate as described throughout and it may not be such a big change if we rely heavily on GHA. (We don't have to decide on this quite yet!)

[cansavvy]

I think moving to a stage and deploy model after we "go live" makes sense. Not only because of the PR process we are discussing here, but also it feels like it will be another checkpoint that will help make sure things are completely ready before being user facing.

Also for items like the survey we will need it to be updated as modules are "live": #277 (comment)

This seems like it should have its own issue at this point so we can discuss details and figure out what that looks like before we "go live".

[cansavvy]

Now that this info has been added to CONTRIBUTING.md, we can close this issue.