Conversation
| <!--- 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 |
There was a problem hiding this comment.
Aren't we essentially replicating the PR Labels already built in to GitHub here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, thanks for reviewing these points 👍 Yup that sounds like a good tickbox to have on the list 😊
|
|
||
| <!--- 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
'Somewhere' == probably in the CONTRIBUTING
There was a problem hiding this comment.
We don't so yeah would be good actually to add that into the CONTRIBUTING page.
There was a problem hiding this comment.
Cool, are you going to add that into this PR or would you see it as a separate contribution?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, good idea. Are you thinking to make the actual change to the CONTRIBUTING tho or leave that for a separate PR?
There was a problem hiding this comment.
Hey @cdpierse so I think this thread is still pending on this PR?
| - [ ] 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice suggestion adding that to the PR
2 participants
Adds a basic PR template for us to use.