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

Update PR template to be more comprehensive and usable #231

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

kelliemac
Copy link
Contributor

@kelliemac kelliemac commented Aug 23, 2024

Description

Updates PR template to be more comprehensive and better organized, so that people are reminded "just-in-time" of the key steps they should be taking before each submitting a PR for review (within an analysis repo).

In particular:

  • I exposed the guidance text that was previously in comments so that people would be more likely to pay attention to and modify it.
  • I copied the checklists from VISC-documentation directly into the PR template, and added/reorganized/simplified to make the checklists more comprehensive but hopefully not too overwhelming.

Related Issues

Related to #228 and in some sense also #119 and #181

Checklist

  • This PR includes unit tests
  • This PR establishes a new function or updates parameters in an existing function
    • The roxygen skeleton for this function has been updated using devtools::document
  • I have updated NEWS.md to describe the proposed changes

@kelliemac kelliemac self-assigned this Aug 23, 2024
@kelliemac
Copy link
Contributor Author

@asatofh and @mayerbry I took a stab at updating the analysis repo PR template to be more in line with the actual workflow I use and tasks I know need to be addressed. hoping this will help the PR process become a little smoother for both PR creators and reviewers. any feedback is welcome!!!

@kelliemac kelliemac marked this pull request as ready for review August 23, 2024 22:45
Copy link
Contributor

@mayerbry mayerbry left a comment

Choose a reason for hiding this comment

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

Thanks Kellie! some super minor comments from me

inst/templates/pull_request_template.md Show resolved Hide resolved
inst/templates/pull_request_template.md Outdated Show resolved Hide resolved
inst/templates/pull_request_template.md Outdated Show resolved Hide resolved
inst/templates/pull_request_template.md Show resolved Hide resolved
inst/templates/pull_request_template.md Outdated Show resolved Hide resolved
kelliemac and others added 5 commits August 26, 2024 10:38
Co-authored-by: Bryan Mayer <39173901+mayerbry@users.noreply.github.com>
Co-authored-by: Bryan Mayer <39173901+mayerbry@users.noreply.github.com>
Co-authored-by: Bryan Mayer <39173901+mayerbry@users.noreply.github.com>
Co-authored-by: Bryan Mayer <39173901+mayerbry@users.noreply.github.com>
@kelliemac
Copy link
Contributor Author

thanks @mayerbry! I incorporated all your suggestions. @asatofh do you want to take a look too, or are you happy for me to merge this PR?

@asatofh
Copy link
Contributor

asatofh commented Aug 26, 2024

thanks @mayerbry! I incorporated all your suggestions. @asatofh do you want to take a look too, or are you happy for me to merge this PR?

@kelliemac The main thing I would love to see is more / better instructions to the reviewer about what specifically to review. I know it's already stated, but often forgotten! Something like (highlighted in bold italics for your review):

Here, describe your changes in detail and provide instruction to the reviewer on areas on which you'd like them to focus. Give a short background on the report, outline important questions or details for the reviewer, and add links to any supporting documents (e.g., protocol presentation). It's helpful to add links to the key files with the unique ID for the commit (a.k.a. the "SHA" or "hash").

@kelliemac
Copy link
Contributor Author

thanks @mayerbry! I incorporated all your suggestions. @asatofh do you want to take a look too, or are you happy for me to merge this PR?

@kelliemac The main thing I would love to see is more / better instructions to the reviewer about what specifically to review. I know it's already stated, but often forgotten! Something like (highlighted in bold italics for your review):

Here, describe your changes in detail and provide instruction to the reviewer on areas on which you'd like them to focus. Give a short background on the report, outline important questions or details for the reviewer, and add links to any supporting documents (e.g., protocol presentation). It's helpful to add links to the key files with the unique ID for the commit (a.k.a. the "SHA" or "hash").

@asatofh I added a section specifically with a checklist for the reviewer. let me know what you think!

@kelliemac
Copy link
Contributor Author

note to self: need to confirm that this PR template is correctly used in new analysis repos. consider manually copying to pre-made analysis repos when ready as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants