tools: Add a pull request template. #394

shahzadlone · 2022-05-03T22:11:04Z

RELEVANT ISSUE(S)

Resolves: #393

DESCRIPTION

Adds the following PR template:

## RELEVANT ISSUE(S)

Resolves #

## DESCRIPTION

Please include a summary of the change and which issue is fixed. If there isn't an issue, then please create one.
Please also include relevant motivation and context. List any dependencies that are required for this change.

### HOW HAS THIS BEEN TESTED?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.

### CHECKLIST:

- [ ] I have commented the code, particularly in hard-to-understand areas.
- [ ] I have made corresponding changes to the repo-held documentation.
- [ ] I have made sure that the PR title adheres to the conventional commit style (subset of the ones we use can be found under: [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)

### ENVIRONMENT / OS THIS WAS TESTED ON?

Please specify which of the following was this tested on (remove or add your own):
- [ ] Arch Linux
- [ ] Debian Linux
- [ ] MacOS
- [ ] Windows

fredcarle

Looks good no me. We can always amend in the future if we find that we need to. I assume this will also be available in the documentation?

AndrewSisley

Got a few comments sorry :P I'm very glad you have taken the time to get this up and running, but have a handful of talking points on some of the specifics. Overall structure is very nice

AndrewSisley · 2022-05-05T18:44:55Z

.github/pull_request_template.md

+- [ ] My code follows the style guidelines of this project.
+- [ ] My changes generate no new warnings.
+- [ ] I have made sure to add label `action/no-benchmark` if benchmarking isn't needed.
+- [ ] I have made sure to add label `action/full-benchmark` if full benchmarking is needed.


suggestion: having tick boxes for the benchmark labels seems like wasteful bureaucracy as it really doesnt matter if I don't add the no-benchmark label if the benches are not needed. Suggest removal of these two lines

AndrewSisley · 2022-05-05T18:45:54Z

.github/pull_request_template.md

+- [ ] New and existing unit tests pass locally with my changes.
+- [ ] My tests cover the newly added code quite reasonably.
+- [ ] My code follows the style guidelines of this project.
+- [ ] My changes generate no new warnings.


todo: If this is part of the template, then we should have no warnings - just build failures. We should convert any existing warnings to errors and remove this line

AndrewSisley · 2022-05-05T18:47:03Z

.github/pull_request_template.md

+
+- [ ] New and existing unit tests pass locally with my changes.
+- [ ] My tests cover the newly added code quite reasonably.
+- [ ] My code follows the style guidelines of this project.


todo: I think this is meaningless - if the build passes, it passes the guidelines (ignoring offensive lang etc). Please remove

AndrewSisley · 2022-05-05T18:48:43Z

.github/pull_request_template.md

+
+## Checklist:
+
+- [ ] New and existing unit tests pass locally with my changes.


todo: I think this is meaningless, if the build passes on our CI then it should pass locally. If it doesnt our CI is broken or they are deving on an unsupported environment (in which case this line is still meaningess)

AndrewSisley · 2022-05-05T18:52:49Z

.github/pull_request_template.md

+## Checklist:
+
+- [ ] New and existing unit tests pass locally with my changes.
+- [ ] My tests cover the newly added code quite reasonably.


todo: I think reasonably is far too subjective to use here and will not contribute to code quality or PR discussions. If we dont have concrete code-coverage requirements (which if we want, the build should fail and this line should be removed), then it is on the author and reviewers to debate this (code-cov is published to PR, and the tests are under review). Suggest removal

AndrewSisley · 2022-05-05T19:00:57Z

.github/pull_request_template.md

+- [ ] I have performed a self-review of my own code.
+- [ ] I have commented my code, particularly in hard-to-understand areas.
+- [ ] I have made corresponding changes to the documentation.
+- [ ] I have ran the linter locally and resolved all the linter errors.


todo: similar to "New and existing unit tests pass locally with my changes." - this doesnt matter and the build should fail in this case - suggest removal

AndrewSisley · 2022-05-05T19:05:48Z

.github/pull_request_template.md

+
+Please specify which of the following was this tested on (remove or add your own):
+- [ ] Manjaro (Linux)
+- [ ] Ubuntu  (Linux)


question: I guess I'll be using this a fair amount, and run Linux Mint which is not quite on the list. I'll happily tick the Ubuntu box though. Might suggest this is too specific though and we either just have Linux, or maybe Debian instead of Ubuntu?

Or maybe a better question: What value does this question/answer provide to author and reviewers? Should this be removed?

I would say this can be valuable to trace if we have discrepancy where something works on the developer's end and not the reviewer or vice versa.

AndrewSisley · 2022-05-05T19:07:08Z

.github/pull_request_template.md

+- [ ] MacOS
+- [ ] Windows
+
+Additional details about the environment:


question: Similar to the OS comment from me - what value does this provide?

AndrewSisley · 2022-05-05T19:11:43Z

.github/pull_request_template.md

+Provide instructions so we can reproduce.
+Please also list any relevant details for your test configuration if relevant.
+Examples:
+- [ ] Added unit test a_test.go


suggestion: I think this is a poor example, as it should be very visible in the PR already and noting it up here is a waste of time and space. Noting any manual testing could be alright, but tbh any changes should be fully covered with automated tests, especially if put forward by external developers (thus making manual tests a bit unnecessary) - I know we have gaps here (e.g. gql intelisense related stuff), but they are very much the exception atm.

AndrewSisley · 2022-05-05T19:12:37Z

.github/pull_request_template.md

@@ -0,0 +1,53 @@
+## Relavent Issue(s)
+
+Resolves #{INSERT-ISSUE-NUMBER}


question: I always use Closes #XYZ - does resolves also link/close the ticket?

yes

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

list of keywords doing that:

close

closes

closed

fix

fixes

fixed

resolve

resolves

resolved

You are welcome to use whatever you like, it's a template / place holder of sorts.

shahzadlone · 2022-05-05T22:53:22Z

@AndrewSisley Appreciate the comments!

I agree with most of your comments, however I feel like perhaps the template so far fails to differentiate between stuff that is just there for reminders and can be removed from the PR description (i.e. optional stuff, and reminders of common tasks that don't need to actually be documented) vs the stuff that is absolute requirement to have.

For example I often forget to add the label action/no-benchmark which then dumps the comparison benchmark reports many times (for many pushes). A reminder for that while PR is opened would be nice, which is what the checklist items are mostly for. Once the self reminders are done the checklist can be removed.

As for what the PR was tested on we can add more options in the PR (template options are editable), but of course nice to have pre-populated options so will add the Linux-mint.

Let me know if you have an idea on how I should differentiate between these optional self reminders vs the fixed template structure elements.

shahzadlone · 2022-05-05T22:55:36Z

Looks good no me. We can always amend in the future if we find that we need to. I assume this will also be available in the documentation?

Yes for sure, just wanted to have something for now. We will have a more in depth CONTRIBUTION_GUIDELINES.md which I believe @orpheuslummis is working towards taking a crack at, that can be referenced for the reminder / optional items.

AndrewSisley · 2022-05-06T00:00:05Z

Let me know if you have an idea on how I should differentiate between these optional self reminders vs the fixed template structure elements.

If they are optional self-reminders I dont think they should be in the organisation's PR template. I don't want to have to bother deleting them (or incorrectly ticking them off) in my PRs, and I dont really want to have to skim past them in PRs from others - it is dead space to me diluting the information density of the description and adding to the cost of development. If everything in here is not mandatory/wanted-by-everyone it will either not be used, or corrupted with inaccuracies and dev-specific variations - similar to a poorly thought out coding standards document.

I also think it increases the odds of pedantic bickering in reviews when people start pointing at 'optional' tick boxes to justify their positions - especially on the boxes with more subjective wording.

That said, this is less problematic if it is optional, in a way that adds zero overhead to those who dont want to use it (I dont know what the UI-flow is for this is), but then that should be made super clear in the name (and could perhaps include your name in the doc/display name).

Ultimately I quite strongly believe that the build itself should take care of any mandatory 'stuff', plus a basic level of competence/professionalism from the author/reviewers RE stuff the build cant handle (like sufficient testing).

shahzadlone · 2022-05-11T09:17:41Z

Let me know if you have an idea on how I should differentiate between these optional self reminders vs the fixed template structure elements.

If they are optional self-reminders I dont think they should be in the organisation's PR template. I don't want to have to bother deleting them (or incorrectly ticking them off) in my PRs, and I dont really want to have to skim past them in PRs from others - it is dead space to me diluting the information density of the description and adding to the cost of development. If everything in here is not mandatory/wanted-by-everyone it will either not be used, or corrupted with inaccuracies and dev-specific variations - similar to a poorly thought out coding standards document.

I also think it increases the odds of pedantic bickering in reviews when people start pointing at 'optional' tick boxes to justify their positions - especially on the boxes with more subjective wording.

That said, this is less problematic if it is optional, in a way that adds zero overhead to those who dont want to use it (I dont know what the UI-flow is for this is), but then that should be made super clear in the name (and could perhaps include your name in the doc/display name).

Ultimately I quite strongly believe that the build itself should take care of any mandatory 'stuff', plus a basic level of competence/professionalism from the author/reviewers RE stuff the build cant handle (like sufficient testing).

I have removed most of the things according to the suggestions, except the environment / OS section. LMK if there is anything else that seems redundant.

AndrewSisley · 2022-05-11T12:22:37Z

I have removed most of the things according to the suggestions, except the environment / OS section. LMK if there is anything else that seems redundant.

😅 Was really hoping some of the others would chime in first, as I think I'm quite 'lean' with stuff like this and dont want to be the only other voice here. Would suggest other reviewers also read through the older commits when reviewing

AndrewSisley · 2022-05-11T12:27:27Z

.github/pull_request_template.md

+## Checklist:
+
+- [ ] I have commented my code, particularly in hard-to-understand areas.
+- [ ] I have made corresponding changes to the documentation.


question: Which documentation? I often prefer to change documentation external to the repo after merge (code/behavior is not finalalized until it is merged) , and I can't tell if this line encompasses that too. 100% agree though if it refers to repo-held documentation such as readme/godocs/comments

suggestion: a more specification indication along these lines:

In light of these changes, I have updated documentation comments. If external documentation should be updated as well, a pull request or issue was created in the relevant repo.

Good suggestion, I was referring to repo-held documentation!

fredcarle · 2022-05-11T17:26:40Z

I have removed most of the things according to the suggestions, except the environment / OS section. LMK if there is anything else that seems redundant.

😅 Was really hoping some of the others would chime in first, as I think I'm quite 'lean' with stuff like this and dont want to be the only other voice here. Would suggest other reviewers also read through the older commits when reviewing

I was happy with the first version and I'm happy with this one. I think I'll have more comments after we start using it. Nothing better than experience for these kinds of things.

shahzadlone · 2022-05-11T21:26:04Z

I was happy with the first version and I'm happy with this one. I think I'll have more comments after we start using it. Nothing better than experience for these kinds of things.

Totes, easier to change in future if we wan to add more stuff. Happy to have a lean one for now and see how it goes.

.github/pull_request_template.md

orpheuslummis · 2022-05-11T12:26:59Z

.github/pull_request_template.md

+
+## Checklist:
+
+- [ ] I have commented my code, particularly in hard-to-understand areas.


thought: 'the code' has less of a connotation of ownership than 'my code'

Good point. Will fix.

orpheuslummis · 2022-05-11T12:33:34Z

.github/pull_request_template.md

+- [ ] I have made corresponding changes to the documentation.
+- [ ] I have made sure that the PR title adheres to the conventional commit style (subset of the ones we use can be found under: [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)
+
+## Environment / OS this was tested on?


suggestion: maybe we can avoid this?

I would like to do a test run with this in and see how it goes. Can be easily removed if we collectively feel it's irrelevant.

orpheuslummis · 2022-05-11T12:36:34Z

.github/pull_request_template.md

+## Checklist:
+
+- [ ] I have commented my code, particularly in hard-to-understand areas.
+- [ ] I have made corresponding changes to the documentation.


suggestion: a more specification indication along these lines:

In light of these changes, I have updated documentation comments. If external documentation should be updated as well, a pull request or issue was created in the relevant repo.

orpheuslummis · 2022-05-11T12:38:01Z

.github/pull_request_template.md

@@ -0,0 +1,27 @@
+## Relavent Issue(s)


suggestion: we can perhaps even remove the title of the section and just start with the "Resolves" ...

How about a dry run, if it seems extra then happy to remove later. Seems like We have gotten quite lean already haha

Whilst I agree with Orpheus here, it does look like if he/I continue to review this there will be nothing left to try out 😅

It's becoming quite clear that the preferred PR template is no PR template at all 🤣. In all seriousness though, a PR template is a necessary evil of large open source projects. Let just start with something and adapt as we go.

codecov · 2022-05-13T00:13:22Z

Codecov Report

Merging #394 (2146b11) into develop (593214a) will increase coverage by 0.66%.
The diff coverage is n/a.

❗ Current head 2146b11 differs from pull request most recent head a8dd792. Consider uploading reports for the commit a8dd792 to get more accurate results

@@             Coverage Diff             @@
##           develop     #394      +/-   ##
===========================================
+ Coverage    64.53%   65.20%   +0.66%     
===========================================
  Files           86       80       -6     
  Lines         9866     9251     -615     
===========================================
- Hits          6367     6032     -335     
+ Misses        2867     2599     -268     
+ Partials       632      620      -12

Impacted Files	Coverage Δ
query/graphql/schema/generate.go	`80.72% <0.00%> (-0.94%)`	⬇️
logging/logger.go	`81.63% <0.00%> (-0.73%)`	⬇️
query/graphql/planner/sum.go	`77.70% <0.00%> (-0.69%)`	⬇️
db/collection_update.go	`42.92% <0.00%> (-0.20%)`	⬇️
db/collection.go	`56.07% <0.00%> (ø)`
datastore/blockstore.go	`45.65% <0.00%> (ø)`
query/graphql/parser/query.go	`75.20% <0.00%> (ø)`
api/http/server.go
core/cid.go
api/http/logger.go
... and 11 more

- RELEVANT ISSUE(S) Resolves: sourcenetwork#393 - DESCRIPTION Adds the following PR template: ``` ## RELEVANT ISSUE(S) Resolves # ## DESCRIPTION Please include a summary of the change and which issue is fixed. If there isn't an issue, then please create one. Please also include relevant motivation and context. List any dependencies that are required for this change. ### HOW HAS THIS BEEN TESTED? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. ### CHECKLIST: - [ ] I have commented the code, particularly in hard-to-understand areas. - [ ] I have made corresponding changes to the repo-held documentation. - [ ] I have made sure that the PR title adheres to the conventional commit style (subset of the ones we use can be found under: [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml) ### ENVIRONMENT / OS THIS WAS TESTED ON? Please specify which of the following was this tested on (remove or add your own): - [ ] Arch Linux - [ ] Debian Linux - [ ] MacOS - [ ] Windows ```

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label May 3, 2022

shahzadlone added this to the DefraDB v0.3 milestone May 3, 2022

shahzadlone requested review from orpheuslummis, jsimnz, fredcarle and AndrewSisley May 3, 2022 22:11

shahzadlone self-assigned this May 3, 2022

fredcarle approved these changes May 5, 2022

View reviewed changes

AndrewSisley requested changes May 5, 2022

View reviewed changes

shahzadlone requested a review from AndrewSisley May 11, 2022 09:18

AndrewSisley reviewed May 11, 2022

View reviewed changes

shahzadlone requested a review from AndrewSisley May 11, 2022 21:21

orpheuslummis approved these changes May 11, 2022

View reviewed changes

AndrewSisley approved these changes May 12, 2022

View reviewed changes

tools: Add github PR template.

a8dd792

shahzadlone force-pushed the lone/tools/add-pr-template branch from 2146b11 to a8dd792 Compare May 13, 2022 00:13

shahzadlone merged commit aa3a7ef into develop May 13, 2022

shahzadlone deleted the lone/tools/add-pr-template branch May 13, 2022 00:17


		## Checklist:

		- [ ] New and existing unit tests pass locally with my changes.

		@@ -0,0 +1,53 @@
		## Relavent Issue(s)

		Resolves #{INSERT-ISSUE-NUMBER}


		## Checklist:

		- [ ] I have commented my code, particularly in hard-to-understand areas.

tools: Add a pull request template. #394

tools: Add a pull request template. #394

Conversation

shahzadlone commented May 3, 2022 • edited Loading

RELEVANT ISSUE(S)

DESCRIPTION

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment • edited Loading

Choose a reason for hiding this comment

AndrewSisley May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orpheuslummis May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone commented May 5, 2022

shahzadlone commented May 5, 2022 • edited Loading

AndrewSisley commented May 6, 2022

shahzadlone commented May 11, 2022

AndrewSisley commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle commented May 11, 2022

shahzadlone commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

shahzadlone commented May 3, 2022 •

edited

Loading

AndrewSisley left a comment •

edited

Loading

AndrewSisley May 5, 2022 •

edited

Loading

orpheuslummis May 5, 2022 •

edited

Loading

shahzadlone commented May 5, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading