search

epiverse-trace / blueprints

Software development blueprints for epiverse-trace
https://epiverse-trace.github.io/blueprints
Other
3 stars 3 forks source link

Expand code reviews section #42

Closed joshwlambert closed 1 year ago

joshwlambert commented 1 year ago

This PR addresses #41.

It extends the Code reviews section of blueprints to include reference to the Tidyteam code review principles, adds explicit points of similarity between Tidyteam and Epiverse-TRACE review practises, and some extra information to aid both Epiverse-TRACE developers or community members how to engage in code reviews.

The text introduced in this PR has caused the Code reviews section to become the largest in the General principles page. I do not have a strong preference as to whether we keep this new information in it's current section or more the new text to a new page in blueprints. This new page might be a good catalyst to work on Policies around Issues/PRs.

joshwlambert commented 1 year ago

I would recommend removing "Prefer small reviews to large ones" as this is now covered in greater depth in the text added in this PR. I have not already made this change as that is text I did not originally author.

Bisaloo commented 1 year ago

My initial idea was to keep the "general principles" pages short and very high-level and detail ideas in separate pages. But I'm now unsure of what's the best approach :thinking:.

@avallecam @annacarnegie, do you have any recommendations in terms of format?

avallecam commented 1 year ago

Grate addition on code reviews, @joshwlambert. @Bisaloo I agree with the idea to keep the "general" short and extend on detailed points on a separate page, similar to the "standards for branching". Also, add a hyperlink to that new page on the box "Read more about this principle in application"

joshwlambert commented 1 year ago

If everyone agrees, I'll move the information I've added to a new page within this PR.

joshwlambert commented 1 year ago

@Bisaloo @avallecam the text is now in a file named code-review.qmd. It is linked in the document index. Happy for either of you to approve or directly merge the changes if your happy with the update or just let me know if there are other changes required before merging.

avallecam commented 1 year ago

It looks good to me, thanks for working on it!