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 PR template with new PR guidelines #390

Closed
bradfordcondon opened this issue May 25, 2018 · 14 comments
Closed

Add PR template with new PR guidelines #390

bradfordcondon opened this issue May 25, 2018 · 14 comments
Assignees
Labels
Community - Discussion Any issue focused on discussion from the community. It does not apply to enhancements. PR Submitted A pull request has been submitted and is awaiting review. Tripal 3 Any issue or pull requuest focused on Tripal 3

Comments

@bradfordcondon
Copy link
Member

Now that we merge PRs according to a guideline, let's add the guidelines and notify submitters of said guidelines in the PR template.

There are some related things to do here but I'll spawn seperate issues/let others do so as we address them.

@bradfordcondon bradfordcondon self-assigned this May 25, 2018
@bradfordcondon
Copy link
Member Author

see #344 for full discussion of guidelines and for adding suggestions/discussion. This issue is just for getting it set up on the repo.

@spficklin
Copy link
Member

Hey @bradfordcondon is this completed now that we have merged #395 ?

@bradfordcondon
Copy link
Member Author

no this is separate. #395 added teh guidelines, this thread would be for a TEMPLATE PR much like you get a template when creating a new issue.

@bradfordcondon
Copy link
Member Author

Some example PR templates.

https://gist.github.com/indiesquidge/85075be518631964a56b75946153ebbf

https://github.com/stevemao/github-issue-templates
https://quickleft.com/blog/pull-request-templates-make-code-review-easier/

From just reading this, the reviewer is sufficiently caught up on what this change is, why it is important and how they should tackle this. Just a little of the author's time can save much more for the reviewer who often lacks context.

@laceysanderson
Copy link
Member

I like this one: https://github.com/stevemao/github-issue-templates/blob/master/questions-answers/PULL_REQUEST_TEMPLATE.md although I would change checklist to “Additional Notes (if any)”

@laceysanderson
Copy link
Member

Throwing up a specific template (because @bradfordcondon shouldn't have to draft them all ;-) ) to start discussion.

<!--- Provide a general summary of your changes in the Title above -->
<!--- See our Contribution Guidelines here:
          https://github.com/tripal/tripal/blob/7.x-3.x/CONTRIBUTING.md -->

## Related Issue
<!--- If it fixes an open issue, please link to the issue here. -->

## Type(s) of Change(s)
<!--- What types of changes does your code introduce? 
         Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] API-specific change (fix or addition to an API function)
- [ ] Updates documentation (inline or markdown files)

## Description
<!--- Describe your changes in detail -->
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, tests ran to see how -->
<!--- your change affects other areas of the code, etc. -->

## Screenshots (if appropriate):

## Additional Notes (if any):
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.

Thoughts?

@bradfordcondon
Copy link
Member Author

bradfordcondon commented May 30, 2018

Result of above.

Related Issue

Type(s) of Change(s)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • API-specific change (fix or addition to an API function)
  • Updates documentation (inline or markdown files)

Description

How Has This Been Tested?

Screenshots (if appropriate):

Additional Notes (if any):

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@bradfordcondon
Copy link
Member Author

bradfordcondon commented May 30, 2018

Thanks @laceysanderson

My starting thoughts:

  • This PR template should be both helpful to new users and relatively painless for dedicated submitters.
  • The template should have some major heading sections
  • The goal of the template is to
    • Make reviewer's lives easier
    • nudge users to submit better PRs

So your draft:

  • Related Issue, yes please. Dont know that it needs a header. there was a template that started with This PR addresses issue #(Issue number here)
  • Types of changes: sure. I'm looking to trim this a bit so this might be what I axe?
  • Description: yes please
  • Testing: yes please. Maybe "Testing" as the header, with one of the comments explicitly: How can the reviewers test the functionality of your code? Please link any example files if necessary.
  • Screenshots : yes please
  • Additional notes: look good.

@laceysanderson
Copy link
Member

I think the types of changes will be a good way for us to determine whether or not it is a minor change and needs only one review or a major change that needs two reviews. It’s just adding an “x” to those that apply so I don’t think it would be a big deal for submitters...

@laceysanderson
Copy link
Member

I like the shorter way to reference the issue !

@bradfordcondon
Copy link
Member Author

bradfordcondon commented May 30, 2018

which one is that lol yours is shorter mine is smaller

think the types of changes will be a good

OK let's keep then :)

@laceysanderson
Copy link
Member

laceysanderson commented May 30, 2018

Oh sorry -typing while walking in ;-) I meant yours. I like that it makes the template feel less intense.

In reference to making it easier for long time submitters, I would be tempted to make the description optional. Perhaps add a comment to the effect of:

If this is a minor change (non-breaking bug fix or documentation update) and you have a descriptive title, this section is optional.

I do worry that we might just never get a description this way but :shrugs:

@laceysanderson laceysanderson added Tripal 3 Any issue or pull requuest focused on Tripal 3 documentation Community - Discussion Any issue focused on discussion from the community. It does not apply to enhancements. labels May 30, 2018
@bradfordcondon
Copy link
Member Author

any other feedback before i make a PR? @spficklin ?

@spficklin
Copy link
Member

spficklin commented Jun 1, 2018

None from me. Thanks @laceysanderson and @bradfordcondon for this. I think it's good for a PR, and since we all agree can just be merged in.

@bradfordcondon bradfordcondon added the PR Submitted A pull request has been submitted and is awaiting review. label Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community - Discussion Any issue focused on discussion from the community. It does not apply to enhancements. PR Submitted A pull request has been submitted and is awaiting review. Tripal 3 Any issue or pull requuest focused on Tripal 3
Projects
None yet
Development

No branches or pull requests

3 participants