Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "How we work" blog post on full package review #335

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

chartgerink
Copy link
Member

@chartgerink chartgerink commented Nov 14, 2024

This is a blog post on "How we work" in Epiverse-TRACE. I figured I'd write one about requesting my first full package review, also as a way to highlight this practice within the ecosystem. It is nothing fancy, but it is always good to highlight the ongoing practices, I figured.

I propose scheduling this blog for December 9th, if agreeable in terms of content.

/schedule 2024-12-09T08:00:00

@chartgerink chartgerink requested a review from Bisaloo as a code owner November 14, 2024 10:17
Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for tourmaline-marshmallow-241b40 ready!

Name Link
🔨 Latest commit 58f9da9
🔍 Latest deploy log https://app.netlify.com/sites/tourmaline-marshmallow-241b40/deploys/673ca6402331170008197085
😎 Deploy Preview https://deploy-preview-335--tourmaline-marshmallow-241b40.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@adamkucharski adamkucharski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting together, some minor suggestions for readability/clarity from me.

posts/20241209full-package-reviews/index.qmd Outdated Show resolved Hide resolved
posts/20241209full-package-reviews/index.qmd Outdated Show resolved Hide resolved
posts/20241209full-package-reviews/index.qmd Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 19, 2024

Merge Schedule
Scheduled on 2024-12-09T08:00:00 (UTC) successfully merged

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments as a reflection on our processes but they are not blocking the publication of this post. Feel free to add some text to clarify if you feel it's necessary or ignore them for now if you prefer.


Our regular code reviews take place on GitHub, using Pull Requests. For full package reviews, we also use Pull Requests in Epiverse-TRACE. This means that all the features from GitHub are available to make suggestions, approve the code, or leave precise comments at a specific line in the code.

What is different, is that all the files are considered new, and part of the review. This includes the files already present in our [package template](https://github.com/epiverse-trace/packagetemplate), but also all test files, documentation, and more. A full package review is usually triggered by an upcoming release, either on GitHub or CRAN.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a comment on the content but a reflection for the future: we could exclude the very first commit, which should correspond to packagetemplate infrastructure, if we wanted in the future.


[Since joining Epiverse-TRACE](../chjh-intro-blog/index.qmd), I participated in many full package reviews, but did not until quite recently initiate my own. For the new package **safeframe**, [I recently requested a full package review](https://github.com/epiverse-trace/safeframe/pull/40). This highlighted two quirks I did not realize before.

First, it feels like doing something that can easily go wrong. I have been using Git for over a decade[^1], but this was completely new to me. Given that git operations can sometimes cause issues, I was hesitant to just run the shell commands. Maybe having a demo video showing it works, will help contributors gain confidence in doing this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we should have a blog post on our backup infrastructure that we could link to 😁


[^1]: My GitHub account is from 2011, but at that time I did not understand what GitHub was or did.

Second, reviewer suggestions are great, but getting them on a full package review involves manual work to include them. I try to leave actionable suggestions directly in the code when I review, so they are easy to accept and integrate into the codebase. With full package reviews, receiving suggestions became slightly annoying. Not because of the reviewer, but because the suggestions have to be manually recreated in a separate branch that will actually be merged. It would be great if we had a way to keep the ease of integrating reviewer suggestions in full package reviews.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the text, I don't know for sure if you are aware that code suggestions can be applied in full package reviews.

The trick is to apply suggestions as you'd normally do in a standard PR. They result in commits in the review branch and at the end of the review you can change the PR target from empty to main and directly merge the changes from the review.

It's another case of "the whole full review process is a hack on top of git and it confuses even advanced git users" but it's quite convenient.

@github-actions github-actions bot merged commit 9a4bd62 into main Dec 9, 2024
10 checks passed
@github-actions github-actions bot deleted the add/how-we-work branch December 9, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants