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 issue + PR templates #66

Merged
merged 3 commits into from
Oct 25, 2021
Merged

Add issue + PR templates #66

merged 3 commits into from
Oct 25, 2021

Conversation

yardasol
Copy link
Contributor

@yardasol yardasol commented Oct 18, 2021

This PR adds issue and PR templates to the repo. Let me know if there is anything you think is missing.

@yardasol yardasol changed the title Update issue templates Add GH templates Oct 18, 2021
@yardasol yardasol changed the title Add GH templates Add issue + PR templates Oct 18, 2021
- [ ] 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.
Copy link
Member

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.

Copy link
Contributor Author

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: -->

Copy link
Member

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)?

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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.

@yardasol yardasol requested a review from munkm October 25, 2021 19:26
Copy link
Member

@munkm munkm left a 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!

@yardasol yardasol merged commit 728db04 into master Oct 25, 2021
@yardasol yardasol deleted the yardasol-issue-templates branch March 9, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants