Add types of review section

[joshwlambert]

This PR adds a section to the Code review (code-review.qmd) chapter of blueprints on best practises for the types of reviews conducted in Epiverse-TRACE.

[netlify]

✅ Deploy Preview for playful-gelato-7892ba ready!

Name	Link
🔨 Latest commit	1e30f4e
🔍 Latest deploy log	https://app.netlify.com/sites/playful-gelato-7892ba/deploys/65b119a5c320600008e79604
😎 Deploy Preview	https://deploy-preview-67--playful-gelato-7892ba.netlify.app/code-review
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[jamesmbaazam]

Thanks for writing this @joshwlambert. Very insightful. Based on our previous conversations, I thought we referred to reviews of small code additions to the code base as "partial reviews" and reviews of the whole code base at any time as full package reviews. I think that distinction is clear to me but the definition of "partial" here could potentially be confusing. I interpret it to mean that a full review only happens when you compare the state of the code base to the initial commit. Is that the definition we're going with?

If we were to go with my definition, then this article could easily be restructured to describe the two ways to do full package reviews: one from the first commit, and one from any commit, where here, we refer to the commit linked to the last release.

What are your thoughts?

[joshwlambert]

@jamesmbaazam I'm not sure I understand your comment.

I thought we referred to reviews of small code additions to the code base as "partial reviews" and reviews of the whole code base at any time as full package reviews

This is the definition I've used when writing the section.

I interpret it to mean that a full review only happens when you compare the state of the code base to the initial commit.

Yes, this is the definition I am going with and have been assuming other people are using. A full review can happen locally (i.e. cloning the repo and reviewing all the code) at any time but, without the full package PR, GitHub does not provide all differences on the Files changed tab of the PR.

[jamesmbaazam]

but, without the full package PR, GitHub does not provide all differences on the Files changed tab of the PR.

This also happens with the approach described in the second paragraph of the partial review section, doesn't it? Wouldn't that therefore be considered a full package (code base) review as well since the reviewer can now comment on all aspects of the code base as if they were touched?

[joshwlambert]

No that is not the case, the partial review between versions only provides the files that have changed since the last version release. In essence it's just a slightly more involved process of creating a standard PR that you would make from a feature branch to main.

[jamesmbaazam]

No that is not the case, the partial review between versions only provides the files that have changed since the last version release. In essence it's just a slightly more involved process of creating a standard PR that you would make from a feature branch to main.

Ah, I see. That's what I was trying to clarify. That makes perfect sense. Thanks. I think the article is a great update to the chapter.

[Bisaloo]

Thanks, this is helpful 🙏

Two potential suggestions for change:

• I would put the new section above the "Recognising contributions in reviews" one
• Rather than making the "targeting first commit" vs "targeting a previous, but not the first commit" distinction, I would rather make a distinction between "pre-release review" / "regular large scale review" / another name and "new feature or bug fix review". Since these reviews should be approached differently and with a different level of scrutiny IMO. In other words, {superspreading} v0.2.0 pkg review superspreading#77 and {superspreading} v.0.1.0 full pkg review superspreading#31 belong to the same category for me. If I understand correctly, this was also what James was saying.

[jamesmbaazam]

• Rather than making the "targeting first commit" vs "targeting a previous, but not the first commit" distinction, I would rather make a distinction between "pre-release review" / "regular large scale review" / another name and "new feature or bug fix review". Since these reviews should be approached differently and with a different level of scrutiny IMO. In other words, {superspreading} v0.2.0 pkg review superspreading#77 and {superspreading} v.0.1.0 full pkg review superspreading#31 belong to the same category for me. If I understand correctly, this was also what James was saying.

Thanks, @Bisaloo. That is what I was saying, but upon further discussion with Josh, I think it would be better to keep the current sections, i.e., full review (the whole code base can be review or commented on) vs partial reviews (review of the current state to an incoming change or previous state), where we can have subsections like reviews of new feature, two states of the package (one version vs another), documentation reviews, etc. The current sections are good because they don't place any conditions on what happens after the review. A "pre-release" review would suggest that there will be a release after the review, but it could just be a routine full review to refactor the code base.

[joshwlambert]

Thanks both for the suggestions. I have made a few minor changes. I don't have anything else to commit, this PR can be merged when ready.