create PR template #853
create PR template #853
Conversation
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -4,5 +4,6 @@ In order to help your fellow volunteers, please note the following suggestions. | |||
|
|
|||
| - [ ] Please read and apply the [Pull Request Guidelines](https://github.com/exercism/docs/blob/master/contributing-to-language-tracks/pull-request-guidelines.md). | |||
| - [ ] Please check the [contributing guide's](https://github.com/exercism/problem-specifications/blob/master/.github/CONTRIBUTING.md#contributing) `Table of Contents` and read the relevant sections. | |||
| - [ ] Lastly, in case this PR modifies `canonical-data.json`, please increment its version as [detailed here](https://github.com/exercism/problem-specifications#test-data-versioning). You can also find more general thoughts on exercise versioning [here](https://github.com/exercism/problem-specifications/blob/master/.github/CONTRIBUTING.md#exercise-versioning). | |||
There was a problem hiding this comment.
please increment its version as detailed here
I strongly dislike the use of "here" because it doesn't make clear what is being linked to.
This is the same idea as that specified in #816 (comment).
Perhaps "please increment its version as detailed in our test data versioning guidelines"
There was a problem hiding this comment.
You can also find more general thoughts on exercise versioning here.
The linked section references track-specific versioning (exercism/discussions#158), not problem-specifications versioning. I don't think the link belongs at all.
There was a problem hiding this comment.
Thanks for the suggestions :-) The 2nd: Done.
About the 1st: Don't all browser show the URL on hovering the mouse pointer over the link text? Because Github-URLs seem very readable to me, I preferred the very version version to duplicating something from the URL in the link text. How about: "please apply our versioning guidelines"
There was a problem hiding this comment.
Don't all browser show the URL on hovering the mouse pointer over the link text?
I imagine so. I don't want to have to hover to find out what a link is about though.
How about: "please apply our versioning guidelines"
seems good!
|
Comment still relevant, even though it is on an outdated. |
|
moving CONTRIBUTING.md is going to break all links to it: (Some of those 38 results are false positives, but still a good number) So then?
|
|
The 5th option: not using And @stkent: My apologies about the weird initial labels! |
|
Nice idea to not use To confirm since I wasn't sure: I tested with a new repository and found that I wouldn't have minded keeping What is your plan with the commits? To squash them all? (If not, I request that the branch be rebased to make the |
I thought about it, but an extra folder for a single file seemed moot. Maybe when
Yes, but as suggested by the track-related PR guidelines 6., I planned to squash my original commits into one, and the "reply-like" later commits into a second one, then force-push that new pair. Or, should I just use |
Hmm, interesting idea. I probably have to see the end result to understand. It might be the case that the first of those two commits moves files into .github, then the second moves them out? Some might argue that this churn is bad to keep in the history. Others say it's good because it is a record that we tried it and didn't like it. I'd have to see it to know what to think. I am strongly against commits whose commit message is just "Address PR feedback" since they are usually not one logical change and usually don't say what the feedback was. This is not to say that your proposed plan was going to have a commit like that, but it does bring up a related point. It means that dividing the changes based on when they were made sometimes doesn't divide them along logical change boundaries. We should be careful that the division splits the commits into one logical change per commit. For me personally, I didn't see a need to split into two commits, but it's very possible I missed a good division into two commits that makes sense (so show us!) |
katrinleinweber
force-pushed
the
847-add-templates
branch
from
22c8273 to
ef6c593
Compare
* omit sub-folder until solution for incoming links is found * focus on PR instructions on details * reflect renaming of repo
katrinleinweber
force-pushed
the
847-add-templates
branch
from
ef6c593 to
d22f38b
Compare
Classical case of "Great minds thing alike." 😉 Strangely this would be "Zwei Dumme, ein Gedanke." in German ("two fools, one thought")... 🤔
Please have a look at |
There was a problem hiding this comment.
usually don't say what the feedback was.
Definitely sufficiently addressed; commit is clear about what is happening!
I think I would personally still not split the commits up in this way because
- The second commit having many bullet points still implies that it is doing more than one thing.
- I find it unlikely that I would want to revert only the second commit (a benefit of splitting commits up). However, we are unlikely to ever want to bisect this repo, therefore this second point is actually unimportant!
(I personally would still squash into one commit. Using Squash and merge would be interesting, since it keeps the history of the two steps (original, then changes after review) in the PR history but they end up as one commit)
On a related note, given this is not critical it doesn't seem necessary for anyone to be too exacting about how the commits end up being split or not. I wouldn't want to spend too much time on it.
Then again maybe unimportant changes are the most opportune time to discuss such matters, because people will have other things on their minds for more involved changes.
Should I rather assume, that the PR will become irrelevant and the commit(s) alone should provide the entire rationale?
It's helpful to have enough information in the commit messages, since the PR and associated discussion are only available with an internet connection. Obviously it might not be possible to capture everything, but some summary. I don't think anything needs to change here since I think that's sufficiently taken care of already; would you agree?
|
Agreed, esp. to @ anyone: Please feel free to do that yourself, if anyone disagrees about waiting. I understand the risk of new merge conflicts being introduced and will work through them if necessary. |
There was a problem hiding this comment.
Thanks @katrinleinweber, I appreciate the time and effort you have put into this, I think you've done a good job of including some of the important things to know about when submitting a pull request.
My opinions:
The changes to the README and the CONTRIBUTING guide should be a separate pull request.
I really don't like the pull request template being used as canonical documentation.
I really don't like the checkboxes in the pull request template due to the way Github treats them as todo lists, this means that we will end up with a lot of pull requests which obscures the ones that really do have pull requests.
The pull request template should include instructions to delete the template text and write a decent pull request message.
|
It seems like I'm disagreeing with @kytrinyx again :( |
|
Hello @Insti, thanks for voicing your opposition. I'm open to discussion :-) Thus: Why do you think that this PR should be split up? Hopefully not only because it touches different files? I feel that the changes across both are logically connected, so even a single commit is arguable (as @stkent has done).
It isn't (yet). It's currently just a link list to such docu. If that should be further streamlined in the future, I'd even argue that 100% of guidelines should be moved into templates. Those (unlike other
I valid risk, but IMHO lower than dealing with PRs that didn't adhere to PR guidelines. Also, GitHub can evolve the template mechanism in the future based on usage data.
Do I understand correctly, that you think
That's the 1st link in the list, so any additional instructions should go there. Unless of course we set out to continue the further streamlining that you seem to have opposed. |
katrinleinweber
dismissed
Insti’s stale review
reviewer concerns addressed here and in …issues/847#issuecomment-312607477. Waiting for replies for another few days would be unduly blocking the expressed preference of other reviewers, so I'll merge.
Adding a pull request template is fundamentally different from rearranging the documentation. Rearranging the documentation is a less controversial activity and can occur whether or not a pull request template is added.
Isn't that what this PR is doing? The documentation of how to write a pull request has been extracted from Having documentation about how to write a good pull request is a good idea. I strongly disagree with adding a pull request template. In the Ruby track where we have been experimenting with pull request templates, my experience is that the quality of the pull requests has not improved, and the templates make contributing and reviewing harder due to all the extra checklists and text (which doesn't get deleted) present in the pull requests. I have read every issue and pull request to this repository for the last year or so, There have been a few occasions where pull request messages have been empty or otherwise unhelpful, but these have been dealt with by replying with specific feedback about how it could be improved and the next pull request made by that person is usually much better. Due to the nature of Exercism, many of the contributors are making their first open source contribution and helping them with specific human guidance is much nicer than a robot response. |
| Take a look at our [pull request guidelines](#pull-request-guidelines). | ||
| You don't need to get it perfect the first time around; we'll work with you to | ||
| get the patch merged. | ||
|
|
There was a problem hiding this comment.
[…] documentation of how to write a pull request has been extracted from CONTRIBUTING.md and added to a separate PULL_REQUEST_TEMPLATE.md document?
@Insti: I have extracted barely more than links (above & below this comment). The actual advice is still at the link targets. How about not deleting those lines here, but duplicating them in the PR template?
There was a problem hiding this comment.
But you have (had) extracted it from CONTRIBUTING.md and moved it into PULL_REQUEST_TEMPLATE.md
I thought that they were still relevant in CONTRIBUTING.md to someone who is reading through that file who might not be making a pull request right now but might want to later.
I would probably suggest creating an explicit "Writing a Pull request" section in CONTRIBUTING.md that contained the relevant information (this may still be a useful change.) and then referring to THAT from the Pull Request Template.
Another possibility would be to greatly simplify the CONTRIBUTING.md file which is linked to in the yellow box, and moving all the information that is not directly related to creating a pull request to a different file.
Is there specific wording in my template suggestion that is robotic? I partially copied what was already there and took care to word the new parts in the same friendly manner. I think that the text being inserted automatically is neither more, nor less robotic than currently the automatic display of the yellow contribution guidelines banner.
OK, thanks for those details :-) I didn't know that and can now understand your criticism better. Given the other two opinions, I still think it's worth a shot. I won't have any hard feelings against reverting my commits if the experience with testing a PR template in this repo for a few weeks recommends that. Then, you'll either be proven right in another repo, or we'll have established a useful tool in this repo. Therefore: shall we agree to merge now, but revisit this discussion in a month or two? |
|
Meta: @katrinleinweber I value your contributions and appreciate that you have invested time in putting together this PR. I believe we both have the best interests of the Exercism project we just have different perspectives on how that can be achieved. ❤️ Back to Discussion: What is the problem that this PR is trying to solve? To me it seems like it is: "Github has Pull Request templates and we are not using them." and my answer to that is it is because we don't need them. What if we were to not merge it and you were to monitor all pull requests for the next two months and identify those for which you thought might have been improved by a pull request template, and what percentage of all requests that is? |
|
Kind of makes one wish one had a way to customise the message that displays on the Open a pull request page rather than "Before you create a pull request, please review the guidelines for contributing to this repository." This template would have had the effect of showing the information in-line, but had the disadvantage of, by default, leaving a message directed toward the submitter in the PR body, whereas the PR body should be directed toward reviewers. Hence why it is necessary to ask the submitter to delete the message, right? I won't be expressing an opinion/speculation on whether these instructions (independent of how they are presented) are likely to improve pull request quality, since I don't know. |
|
@petertseng's suggestion actually might be something for @kytrinyx to bring up with the GitHub team, and we could have it perhaps ? |
|
I don't have any suggestive powers at GitHub, but if I find the right people I'll bring it up. |
|
One of my colleagues said:
|
|
I'm also asking myself, whether a PR template would prevent the yellow banner from appearing? I agree with @stkent that tweaking that banner would be most desirable. Hopefully, a GH update will deliver such improvement. @Insti No worries <3 This is extremely educational. However, it would be more so, if you replied to my answers to some of your minor counter-arguments ;-) I agree that our main disagreement will probably only be resolved by observing PR quality over time, with our without merging this. Which I BTW will not do; somebody else please decide that, or close this. I'll stick to smaller topics. |
|
I've gone back and forth on this one, and I think that I'd like to suggest the following:
I think automation is probably a more effective approach than asking people to read stuff. Especially if we're going to complain about stuff :) People tend to not take bots personally. |
Sure, which ones in particular? |
|
https://github.com/exercism/problem-specifications/pull/853/files#r125525332 & the "robotic" part of #853 (comment) |
No, not particularly. What you wrote was fine and friendly. The checkboxes maybe, as every PR would need to tick them off to acknowledge that they'd done them, rather than just reading them. The PR template also applies to experienced committers who HAVE read these documents (and in many cases wrote them.) I was using this as a general principle about how we might want to treat contributors. |
|
Ok, I hope I've answered those 2 satisfactorily, let me know if there's anything else, |
All exercises: Update generators and regenerate tests
As discussed in #847. I suggest to keep the general discussion about consolidating/streamlining contribution guides there, and continue here only with specific comments about these commits.
Such as 😉 : The template is a bit longer than I hoped (reproduced below). PLMK if you consider something worth dropping or adding.
Preview of the PR template (links modified here to work within without merge)
Hello, and thanks for contributing to Exercism!
In order to help your fellow volunteers, please note the following suggestions. Feel free to
[x]-check or remove them afterwards. UsePreviewto activate hyperlinks ;-)Table of Contentsand read the relevant sections.canonical-data.json, please increment its version as detailed here. You can also find more general thoughts on exercise versioning here.You don't need to get this perfect right away. We'll help getting your pull request merged :-)