Add types of review section

joshwlambert commented 8 months ago

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[bot] commented 8 months ago

Deploy Preview for playful-gelato-7892ba ready!

Name	Link
Latest commit	1e30f4e32172784c0d7bc641df53480c85d71aad
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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

jamesmbaazam commented 8 months ago

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 commented 8 months ago

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.

epiverse-trace / blueprints