Updating PR template, doc links to PR info #324
Conversation
|
Nice, it's very clean. You could link directly to the relevant section of the Resource Index : https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers, both in the PR template and the documentation. |
|
@phil-blain Thanks, made that change. :) |
|
@duvivier, I have put a proposal for an alternative template at cheyenne:/gpfs/u/home/tcraig/PULL_REQUEST_TEMPLATE. I think what you have is good. This is just a proposal for an alternative that is a little cleaner still (maybe too clean). I think this is largely a personal preference so leave it to you to incorporate as much or little as you like. One thing I would say is that I think the short summary sentence should come before Developer, but I can see both working. |
|
@apcraig I modified based on your suggestions to make it more streamlined. |
|
Thanks @duvivier, I am interested what other folks feel about this style vs the earlier one. I can go either way. One other issue is the question of checking boxes that have check boxes under them. In the PR above, for instance, the "does this have dependency" box is not checked but a box under it is. I wonder if we should get rid of the top level box? Do we care if each check box is ticked if boxes below are? Not sure exactly the best way to deal with that, or maybe we should worry about it. |
|
@apcraig I am fine with streamlining it. I think in general the longer/more complicated something is the more people ignore bits of it. As for the check boxes under check boxes, I'm not sure. I guess I figured the top level check boxes are for people to go through themselves to be sure they're doing each step. The sub level of check boxes is more for us to know what tests are needed, who should be a reviewer, etc. I'm interested in @eclare108213 or @proteanplanet thoughts too. |
|
Just a minor correction. In the first sentence, it needs to be "For details about" or "For detailed information about". |
There was a problem hiding this comment.
I like this, in general. My suggestions:
Put checkboxes in front of requests, and not in front of questions. The questions should all be yes/no or multiple-choice (and they are). I'd put the request for test result info immediately before the code-changes question above it, and I would correct the grammar in that one, somehow. It needs a question mark... How about "How much does the output differ from the unmodified code?" Just a suggestion.
|
@eclare108213 I've made these changes. You can see the result above - I've updated the PR request formatting for this PR. Let me know what you think. I'm not sure I like the bullets with the check boxes, but I don't feel really strongly about it. |
|
I think this is a very nice improvement over what we had a week or two ago. It's nice to see things continue to improve with respect to process, documentation, and organization! I can see going with all tick boxes, all bullets, or a mix at the lowest outline level. I don't have a strong preference, but maybe all bullets could be tried with the tick boxes only be used when there is a clear and limited set of options. Again, I don't have a strong opinion. Thanks! |
|
Alright, I experimented with trying to add bullets before the check boxes and it isn't a straight forward process. So... I propose we go ahead and merge this. We can modify it later if we don't feel it's working smoothly. I'll go ahead and start a similar PR for Icepack. |
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Clean up the PR template process and links to PR info in documentation.
Alice DuVivier
N/A, will only need Travis CI test
I've noticed that it can be hard to follow the PR template and people forget things. I've tried to just create a checklist and make sections so we can better flag who needs to pay attention to each PR and if all steps have been followed. Suggestions welcome!