-
Notifications
You must be signed in to change notification settings - Fork 16
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 issue + PR templates #66
Conversation
- [ ] My change requires a change to the documentation. | ||
- [ ] I have updated the documentation accordingly. | ||
- [ ] I have added tests to cover my changes. | ||
- [ ] All new and existing tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because devs will need to run some tests locally that aren't run with CI, this should be expanded to two checkboxes, one related to whether CI passes and one whether the serpent-requiring tests pass locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in most recent commit
|
||
## Types of changes | ||
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about splitting this list into two separate lists: one with the descriptors for the types of changes and requirements (i.e. all of the "optional" checkboxes), and one with the checkboxes that are required for merging (i.e reading the contributing doc, making sure tests pass)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in most recent commit.
|
||
## Checklist for Reviewers | ||
|
||
Reviewers should use [this link](/manual/guides/pull_requests) to get to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this link will complete because it's relative to the arfc website. You may need to add /saltproc
before /manual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled this from the arfc.github.io PR template. Using that link doesn't work there either (I'll open an issue for it in that repo). The link is supposed to go to the PR Review Checklist on our group website. I've changed the link in this template to point to that page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes you made look good!
This PR adds issue and PR templates to the repo. Let me know if there is anything you think is missing.