Update Pull Request Template, add question about updating Icepack

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Update PR template
• Developer(s):
  apcraig
• Suggest PR reviewers from list in the column to the right.
• Please copy the PR test results link or provide a summary of testing completed below.
  No tests run
• How much do the PR code changes differ from the unmodified code?
  • bit for bit
  • different at roundoff level
  • more substantial
• Does this PR create or have dependencies on Icepack or any other models?
  • Yes
  • No
• Does this PR add any new test cases?
  • Yes
  • No
• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes
    • No
• Please provide any additional information or relevant details below:

Add questions about whether the Icepack submodule has been updated.

[phil-blain]

I think we should be specific and ask the second question I suggested in #753 (comment).

[phil-blain]

Suggested change

	- [ ] Yes
	- [ ] Yes, does it point to a commit in Icepack's `main` branch ?
	- [ ] Yes
	- [ ] No

[apcraig]

I sort of disagree. I think that's getting too deep in the weeds and folks that have erroneously included Icepack changes probably won't understand. I think even the first question is unlikely to be answered correctly all the time when folks make a mistake. I think the main point of the new question is to raise awareness that Icepack is relatively easy to change in CICE and to remind us reviewers to double-check.

Maybe what we really need is a review template that reviewers have to fill out.

• Is Icepack updated
• Is the documentation updated
• Does the documentation look OK
• Is testing adequate
  ...

[dabail10]

I think the confusing thing is that we always point to a "hash" of Icepack. So, this might be confusing mention main. Otherwise I do like this.

[eclare108213]

Rather than asking this as a question, it could just be noted, e.g.
Does this PR update the Icepack submodule? If so, it must point to a hash in Icepack's main branch.
yes/no

[phil-blain]

I think that's a good idea !

[apcraig]

OK, I added an extra sentence.

[eclare108213]

please change 'should' to 'must'

[apcraig]

must seems a little too rigid, but I'll make the change.

[apcraig]

@phil-blain, are you OK with the current state of changes and approve?

[phil-blain]

Also, while we are modifying the PR template, I would take the time to adjust the first question:

Short (1 sentence) summary of your PR:

This question is mostly useless in my opinion, since the title of the pull request should be exactly that: a one sentence summary of the PR, in the imperative mood (as if giving an order to the codebase to improve itself). So I think the first bullet could be changed like so:

diff --git a/./.github/PULL_REQUEST_TEMPLATE.md b/./.github/PULL_REQUEST_TEMPLATE.md
index 6256f9b..768aeee 100644
--- a/./.github/PULL_REQUEST_TEMPLATE.md
+++ b/./.github/PULL_REQUEST_TEMPLATE.md
@@ -4,8 +4,7 @@ please refer to: <https://github.com/CICE-Consortium/About-Us/wiki/Resource-Inde


 ## PR checklist
-- [ ] Short (1 sentence) summary of your PR:
-    ENTER INFORMATION HERE
+- [ ] The title of your PR (above) is a one sentence summary of your PR, in the imperative mood.
 - [ ] Developer(s):
     ENTER INFORMATION HERE
 - [ ] Suggest PR reviewers from list in the column to the right.

I would also take the time to remove the second checkbox, since the developers of the PR is already recorded by Git in the commit authorship information:

diff --git a/./.github/PULL_REQUEST_TEMPLATE.md b/./.github/PULL_REQUEST_TEMPLATE.md
index 6256f9b..691c615 100644
--- a/./.github/PULL_REQUEST_TEMPLATE.md
+++ b/./.github/PULL_REQUEST_TEMPLATE.md
@@ -6,8 +6,6 @@ please refer to: <https://github.com/CICE-Consortium/About-Us/wiki/Resource-Inde
 ## PR checklist
 - [ ] Short (1 sentence) summary of your PR:
     ENTER INFORMATION HERE
-- [ ] Developer(s):
-    ENTER INFORMATION HERE
 - [ ] Suggest PR reviewers from list in the column to the right.
 - [ ] Please copy the PR test results link or provide a summary of testing completed below.
     ENTER INFORMATION HERE

[apcraig]

I don't know that the PR template is supposed to just supplement information that's already available. I think it's OK that the PR template stands alone to summarize the PR. Yes, the title and short summary should basically be the same (but they often aren't) and the developers don't always include just people that made commits. Others may have contributed discussion or ideas. So I think it's good to explicitly site the contributors even if it's mostly redundant with information elsewhere.

Ultimately, the commits, PR title, PR template (and other stuff) are going to be somewhat redundant. But part of the point of doing that is to not have to enforce a strict syntax/style throughout. One could argue, we don't need a PR template because it should all be encapsulated in the commits. But that's not practical. The commit process is difficult to control and some are more discipline than others. Often commits are made, then undone, refactoring several times. Ultimately, the PR template is a point where all that information can be summarized and provided in a final, clean form.

[eclare108213]

I like having the PR summary separate from the title. Personally, I prefer the title to be very cursory (a few words, understandable at a glance), and the summary to include a bit more information or explanation. Then details go at the bottom.

In the zenodo release records, I include commit authors but not others listed in the PRs. Maybe I should pay more attention to that? Almost everyone is a consortium team member and listed on the releases regardless of commit activity.

[eclare108213]

please change 'should' to 'must'