Skip to content

Add intial version of PR template #32

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add intial version of PR template #32

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d502866
Add intial version of PR template
cdpierse Apr 1, 2021
46f13f1
Update PR template
cdpierse Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<!--- Please provide a general summary of your changes in the Title above -->

# PR Description

<!--- Describe the changes your PR makes in detail -->

References issue: # (ISSUE)

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## Tests and Coverage

<!--- 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. -->

## Types of changes
Copy link
Owner

@Voyz Voyz Apr 2, 2021

Choose a reason for hiding this comment

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

Aren't we essentially replicating the PR Labels already built in to GitHub here?

Copy link
Collaborator Author

@cdpierse cdpierse Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I these have a little bit of overlap but still are okay on their own, I think it's good to have it in the PR's written documentation rather than just hope that the correct label has been applied aswell.

Copy link
Owner

@Voyz Voyz Apr 15, 2021

Choose a reason for hiding this comment

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

Amm... is it only a bit of overlap though? 😅

These are the pairs I see here between the tickbox and a label

Bug fix - Bug
New feature - Enhancement (arguably we could rename this if we feel 'New Feature' sounds more descriptive - I kinda agree)
Docs - Documentation

The only one that we don't have a label for yet is 'Breaking change'. That's 3/4 of them already being a label, we could easily add 'Breaking Change' as another one. What benefits do you see of having these as tickboxes as opposed to labels? I feel that labels are more useful as they show on the list of Issues or PRs, making it easier to browse the respective history in the future and search for specific labels - while these tickboxes would be hidden in the PR content.

Also - if we're 'hoping' they'd put the right label on (which actually I think we should be doing - not them - as we understand the scope of the project better), we'd have to 'hope' that they'd put the X in the right tickbox just as much, wouldn't we? In my experience with GitHub I've seen a ton of people completely or partially ignore the templates - as much as I wished it was otherwise!

Copy link
Collaborator Author

@cdpierse cdpierse Apr 19, 2021

Choose a reason for hiding this comment

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

Yep you're right. We'll stick with the labels, what might be nice to do for this section then is to ask "Have you assigned the appropriate Github labels to this PR" seems to be the best of both worlds.

Copy link
Owner

@Voyz Voyz Apr 19, 2021

Choose a reason for hiding this comment

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

Cool, thanks for reviewing these points 👍 Yup that sounds like a good tickbox to have on the list 😊


<!--- 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)
- [ ] Docs (Added to or improved Databay's documentation)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

## Final Checklist:
Copy link
Owner

@Voyz Voyz Apr 2, 2021

Choose a reason for hiding this comment

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

I'd maybe suggest adding something about code coverage here too. For example:

[ ] I've run coverage on the project
The coverage is: [INPUT COVERAGE PERCENTAGE HERE]

We could also add how to run coverage (and tests) into CONTRIBUTING, and link to it here.

Copy link
Collaborator Author

@cdpierse cdpierse Apr 14, 2021

Choose a reason for hiding this comment

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

Nice suggestion adding that to the PR


<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->

- [ ] My code follows the code style of this project.
Copy link
Owner

@Voyz Voyz Apr 2, 2021

Choose a reason for hiding this comment

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

Do we say what the code style is anywhere? If not we should write it somewhere. And in either case we could link to that place here.

Copy link
Owner

@Voyz Voyz Apr 2, 2021

Choose a reason for hiding this comment

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

'Somewhere' == probably in the CONTRIBUTING

Copy link
Collaborator Author

@cdpierse cdpierse Apr 14, 2021

Choose a reason for hiding this comment

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

We don't so yeah would be good actually to add that into the CONTRIBUTING page.

Copy link
Owner

@Voyz Voyz Apr 15, 2021

Choose a reason for hiding this comment

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

Cool, are you going to add that into this PR or would you see it as a separate contribution?

Copy link
Collaborator Author

@cdpierse cdpierse Apr 19, 2021

Choose a reason for hiding this comment

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

What I'll do is make the wordscode style a link to the CONTRIBUTING page where we can add the code style in. It should hopefully encourage them to read that too.

Copy link
Owner

@Voyz Voyz Apr 19, 2021

Choose a reason for hiding this comment

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

Cool, good idea. Are you thinking to make the actual change to the CONTRIBUTING tho or leave that for a separate PR?

Copy link
Owner

@Voyz Voyz Jun 18, 2021

Choose a reason for hiding this comment

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

Hey @cdpierse so I think this thread is still pending on this PR?

- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.