#2 #6 #16 code review draft work added

_quarto.yml

@@ -4,6 +4,7 @@ project:
 book:
   title: "DaSL Contributor Guide"
   reader-mode: true
+  bread-crumbs: false
   author:
     - Scott Chamberlain
   page-footer:
@@ -16,7 +17,12 @@ book:
     - index.qmd
     - contributing.qmd
     - style.qmd
-    - codereview.qmd
+    - part: codereview.qmd
+      chapters:
+        - codereview-dasl.qmd
+        - codereview-external.qmd
+        - codereview-general.qmd
+        - codereview-guidelines.qmd
     - packagedocs.qmd
     - maintenance.qmd
     - security.qmd

codereview-dasl.qmd

@@ -0,0 +1,50 @@
+# DaSL internal {#sec-review-dasl}
+
+## High level approach
+
+At a high level we use the following approach:
+
+- Code review whenever possible - review is generally not required for every code change
+  - As a team grows, add in more code review
+  - Some projects in the WILDS may have changess pushed directly to main, while others may follow a pull request only approach
+- Follow our code review guidelines (@sec-review-guidelines) (TO BE AGEED UPON)
+- Testing must be used
+  - With a small team testing is crucial to ensuring software does what it is expected to do, AND facilitates code changes in the future
+  - When making a code contribution, make sure it is covered by existing tests, or write a new test if not
+- Automate the boring work / Have bots tell us to follow our own rules
+  - Styling
+  - Linting
+  - Running tests/checks/etc
+
+The approach above may change as the portion of the team grows that works on WILDS projects. In particular, our guidelines around human code review should change as we have more team members. For example, where we strive for just one reviewer of a pull request now, if we add a few new team members we may expect two reviewers.
+
+Another factor that may change the above approach is if a project gains significant external contributors - making it possible to do more human code review. We cannot however expect rapid response times for external contributors.
+
+## Patterns of collaboration
+
+The tidyverse team has a nice chapter in their code review guide on [patterns of collaboration](https://code-review.tidyverse.org/collaboration/). The exact scenarios may not apply here, but in general we need to consider different scenarios of how we work together, who the players are and what we can expect from them.
+
+### Close-knit collaboration
+
+On some projects there may be 2 or more contributors that are heavily involved in work on the project. In these cases, code review of PRs should be swift.
+
+### Solo developer
+
+There will be many cases within the WILDS where there is only one person working on the project - and there isn't anyone available within DASL to review code. In these cases, tests and automation are particularly important.
+
+These projects should strive to get other DASL staff to review code, but turnaround times on reviews are not likely to be fast.
+
+### Understudy
+
+This could be a more junior DASL staff member, or an external contributor that's still "learning the ropes". In these cases PR reviews should be swift to make sure the understudy is getting feedback quickly so they know the lead maintainer is interested in their work.
+
+### External
+
+External contributions may be one-off drive by contributions from someone that may not contribute again, or contributions from a regular external contributor. Although DASL staff need not fully address these immediately, we should make every effort to reply quickly so the external contributor knows a human at DASL is aware of their contribution and will address it soon.
+
+## What else?
+
+::: {.callout-warning}
+What else should we cover in this chapter?
+:::
+

codereview-external.qmd

@@ -0,0 +1,23 @@
+# DaSL external {#sec-review-external}
+
+By external we are referring to anyone that is not a DASL employee.
+
+## Follow code review guidelines
+
+Follow our code review guidelines (@sec-review-guidelines) (TO BE AGEED UPON)
+
+## Discuss first
+
+In most cases it's a good idea to start a discussion before opening a pull request.
+
+You can do this by opening an issue in the relavant GitHub repository, starting a discusison in the [FH-Data Slack](https://fhdata.slack.com/) or emailing the maintainer. The best option is opening an issue as that keeps the discussion of the code paired with the code.
+
+If you open an issue, there should be an issue template that will give instructions.
+
+Make sure to include a [reproducible example](https://www.tidyverse.org/help/#reprex)!
+
+## Open a pull request
+
+After discussing a change, feature request, bug, or soemthing else, open a pull request.
+
+ADD MORE TEXT ABOUT DOING A PR ...

codereview-general.qmd

@@ -0,0 +1,10 @@
+# General {#sec-review-general}
+
+We have code review guidance for labs at <https://hutchdatascience.org/code_review/>. The site contains higher level code review guidance broken down by lab roles, including lab leader, lab manager and lab developer.
+
+If you want lower level code review guidance, the [Advanced Reproducibility in Cancer Informatics](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/) course has two chapters that will be helpful:
+
+- [Engaging in Code Review - as an author](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/engaging-in-code-review---as-an-author.html)
+- [Engaging in Code Review - as a reviewer](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/engaging-in-code-review---as-a-reviewer.html)
+
+The Tidyverse team at Posit (that makes the R packages `dplyr`, `purrr` and so on) has made public their code review principles. Check it out at <https://code-review.tidyverse.org/>

codereview-guidelines.qmd

@@ -0,0 +1,70 @@
+# Guidelines {#sec-review-guidelines}
+
+Adapted from:
+
+- ThoughtBot's Playbook section on [code review](https://github.com/thoughtbot/guides/tree/main/code-review) under a [CC Attribution License](https://creativecommons.org/licenses/by/3.0/)
+- Monica Gerber (MG)
+
+## Everyone
+
+- Accept that many programming decisions are opinions
+  - Discuss tradeoffs, which you prefer, and reach a resolution quickly.
+- Offer positive comments (MG)
+  - What did you learn? What did you think was neat?
+- Ask good questions; don't make demands
+  - "What do you think about naming this function `fetch_user`?"
+- Good questions avoid judgment and avoid assumptions about the author's
+  perspective
+- Ask for clarification
+   - "I didn't understand. Can you clarify?"
+- Avoid selective ownership of code
+   - "Mine", "not mine", "yours"
+- Avoid using terms that could be seen as referring to personal traits
+  - "Dumb", "stupid".
+  - Assume everyone is intelligent and well-meaning.
+- Be explicit
+   - Remember people don't always understand your intentions online.
+- Be humble
+   - "I'm not sure - let's look it up."
+- Don't use hyperbole
+   - "Always", "never", "endlessly", "nothing"
+- Don't use sarcasm
+- Talk synchronously if there are too many "I didn't understand" or "Alternative solution:" comments
+  - Chat, screen-sharing, in person
+  - Post a follow-up comment summarizing the discussion.
+- If you learned something new, share your appreciation
+   - "I did not know about this. Thank you for sharing it."
+
+## Having Your Code Reviewed
+
+- Be grateful for the reviewer's suggestions
+  - "Good call. I'll make that change."
+- Be aware that it can be challenging to convey emotion and intention online
+  - You may want to consider using labels to convey intention and tone.
+- Explain why the code exists
+  - "It's like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?"
+- Extract some changes and refactoring into future tickets/stories
+- When making visual changes, include screenshots or screencasts to show the effect of the changes
+  - You may want to consider before/after screenshots or screencasts whenever applicable.
+- Seek to understand the reviewer's perspective
+- View each comment as an opportunity to learn (MG)
+- If you're someone who says "sorry" frequently, think about removing this from your language (MG)
+- You can ask for clarification (MG)
+- It's OK to disagree. It's helpful to first look for things that you agree with and explain why you disagree (MG)
+
+## Reviewing Code
+
+- Communicate which ideas you feel strongly about and those you don't
+- Identify ways to simplify the code while still solving the problem
+- If discussions turn too philosophical or academic, move the discussion offline
+  - In the meantime, let the author make the final decision on alternative implementations.
+- Offer alternative implementations
+  - But assume the author already considered them.
+  - "What do you think about using a custom validator here?"
+- Seek to understand the author's perspective
+- Remember that you are here to provide feedback, not to be a gatekeeper
+- When suggesting changes using the "Add a suggestion" feature (look for this icon: {{< iconify octicon file-diff-24 >}}):
+  - Communicate clearly which lines you suggest adding/removing
+  - Test the suggested changes to validate it works whenever possible
+    - When not possible, let the pull request author know that you did not test the suggestion
+  - Provide some context to let the author know why you're suggesting the change

codereview.qmd

@@ -1,17 +1,30 @@
 # Code review {{< iconify fa6-solid pencil >}} {#sec-review}
 
-Code review ....
+As code review is a big topic, we will only discuss here what we strive for, what we'd like our community to do, and point people to relavant information. We're always open to feedback (see *Edit this page* to the right)!
 
-## DaSL interal guidelines
+MORE STUFF ABOUT HIGH LEVEL CODE REVIEW ...
 
-still discussing this internally ...
 
-## Community guidelines
+## Scope
 
-We have code review guidance for labs at <https://hutchdatascience.org/code_review/>. The site contains higher level code review guidance broken down by lab roles, including lab leader, lab manager and lab developer.
+The scope of this guide is for work within the WILDS GitHub organization (<https://github.com/getwilds/>). In  chapters within **Code review** we have the following guidelines:
 
-If you want lower level code review guidance, the [Advanced Reproducibility in Cancer Informatics](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/) course has two chapters that will be helpful:
+Contributions to the WILDS:
 
-- [Engaging in Code Review - as an author](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/engaging-in-code-review---as-an-author.html)
-- [Engaging in Code Review - as a reviewer](https://jhudatascience.org/Adv_Reproducibility_in_Cancer_Informatics/engaging-in-code-review---as-a-reviewer.html)
+- from within team DASL (@sec-review-dasl)
+- from non-DASL folks, and (@sec-review-external)
+
+And
+
+- advice for broader Fred Hutch community (@sec-review-general)
+
+## Infrastructure
+
+We use a number of helpful tools to facilitate code changes, including code review:
+
+- Contributing instructions in the file `CONTRIBUTING.md`
+- Pull request instructions in a file `pull_request_template.md`
+- Issue instructions in a file `issue_template.md`
+
+Read more about these files in the section @sec-conventions-repos.
 

conventions.qmd

@@ -22,7 +22,7 @@ In all repositories in WILDS we will include mention of our employer. The follow
 
 For other types of WILDS repositories, perhaps just add a mention to the README.
 
-## GitHub Repositories
+## GitHub Repositories {#sec-conventions-repos}
 
 ### Issue labels
 
@@ -46,3 +46,31 @@ WILDS badges are maintained in a GitHub repository at <https://github.com/getwil
 
 An example badge: [![](https://getwilds.org/badges/badges/stable.svg)](https://getwilds.org/badges/#stable)
 
+### Contributing instructions
+
+All WILDS repos should have a `CONTRIBUTING.md` file. Although this file could live in many different places in a repository, in WILDS repositories it should live in the `.github/` directory.
+
+See also [GitHub's docs on CONTRIBUTING.md](https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors).
+
+### Pull request templates
+
+All WILDS repos should have one or more pull request templates. These help contributors include all the relavant information, and perform any required tasks, before submitting a pull request.
+
+See also [GitHub's docs on pull request templates](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository)
+
+### Issue templates
+
+All WILDS repos should have one or more issue templates. These help contributors include all the relavant information, and perform any required tasks, before submitting an issue.
+
+See also [GitHub's docs on issue templates](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/manually-creating-a-single-issue-template-for-your-repository)
+
+
+::: {.callout-warning}
+Question for the team: Should we have organization wide files (contributing, issues, PRs) for all WILDS repos? Or some other plan?
+:::
+
+
+### Reproducible examples
+
+We strongly encourage all repos that are R packages or projects/etc. to request a "reprex" - a reproducible example. The R package [reprex](https://www.tidyverse.org/help/#reprex) makes it easy to do.
+