Issue and Pull Request Templates, with addendums #1187
Conversation
The function of this method is to defer the logic of generating a commit message to template.go, and eventually remove that logic from commands/pull_request.go Also adds tests for this new method, which also modifies the fixtures/test_repo.go file to accomidate a test repo with a `.github` PR dir.
This allows the github.GeneratePRTemplate() function to autofill the pull request title for a single commit pull request with either: * The entire commit message, if it is a single line commit message * The first line of the commit message, if it is a multi-line commit message As far as the code is concerned, both cases above are interpreted in the same fashion, but the expectations of the user are defined with each of the new test cases in github/template_test.go FYI, this commit message would be an example of a multi line commit message :)
Implements the GeneratePRTemplate in pull_request.go instead of the custom logic that previously existed in the file. Addresses: * Title being defaulted as the first line of the template * Commit messages not defaulting as the title only properly
For the longest time I was assuming that this was a whitespace change with `git diff`, even though I had fixed it when building this a few weeks back, so I decided to not bother adding it to the previous commits. When I removed it and re-ran the tests, things stopped working... oops :)
These typos were hiding failing tests (i.e., two mistakes were canelling each other out and making the tests pass accidentally). The tests *should* have been failing because the new addGitHubTemplates function was writing GitHub templates to the wrong place in the test repo fixture. They need to be written inside the repo, not in the same directory as the repo.
|
/cc @mislav Bump :D |
| message = fmt.Sprintf(message, project.Name) | ||
|
|
||
| if template := github.GetIssueTemplate(); template == "" { | ||
| message = fmt.Sprintf(message, "", project.Name) |
There was a problem hiding this comment.
It seems that this if/else construct is unnecessary. The logic getting executed is identical regardless of the fact whether the template string is empty or not.
There was a problem hiding this comment.
You're right. I'll clean that up.
|
Thanks @jez. This looks good, but I'm still wondering about desired behavior for displaying our usual commented-out text like “# Write a message for this issue. The first line is the title…” I don't think this text should get displayed at all when there is an issue template. But that opens more questions for to do in the case of PRs. Right now, without a PR template, the text displayed gives you information about which branch is the PR going to get opened against, and provides you with a changelog of all commits (all commented-out). This is useful information. If there is a PR template, we can make all this go away, but that would also eradicate some of this useful information that I just pointed out. Thoughts? |
I've been using my patched fork of hub at work for the past week, and I actually like it the way it is. I frequently write detailed commit messages, and I like how the commits show up so I can copy/paste into my pull request message.
Even though these are power users (using GitHub, and using the command line, and using configured templates), I think there's value in the user interface aspect of the text. Git has had Those are my thoughts on the issue. If you'd like it implemented differently, I'm more than happy to oblige. My core concern is for hub to support templates in the first place, as it's a feature many people will enjoy. |
|
My 2 cents on the topic (not that I am a heavy code contributor in this endeavour): I pretty much agree that the change-log of commits, and base and head info should stay in with the template. I found myself referencing that when fleshing out templates myself, and also for double checking that my changes were going to the right spot before I sent it off, and was able to abort by deleting the entirety of the file if that wasn't the case. This info should stay in my humble opinion. If we were format the comments based on the presence of a template, I think we might want to consider moving the https://github.com/github/hub/blob/master/commands/pull_request_tpl.go Or vice versa, as there would need to be a bit of logic mixed with the two when dealing with the rendering of the template and removing certain pieces of text based on whether a template was present or not. Alternatively, do we treat these like they are already being done with single commit PRs, where a message is pre-filled from the commit message, and that info is displayed as well for that regardless? I am personally not bothered by being reminded what needs to be done in the Pull Request message, and wouldn't mind leaving it as is. It is also good for people not used to using the pull request templates via hub (or even knew they were a feature of github/hub) to be reminded how it is formatted when editing the template it in your editor. What threw me off the first time I used @ganmacs original implementation was the fact that it put in the template right at the beginning of the buffer, and not until I committed my pull request did I realize that the first line was not the PR title, but the beginning of the template, so everything was formatted weird (hence my infamous P.S. Just noticed that P.P.S @jez Did not know about |
That's kinda wonky. Fixed.
😀 |
|
@mislav I think it's back in your court right now. |
|
Thanks for your opinions! |
This adds a standard way of formatting a pull-request and guiding a contributer to the prefered format and level of detail for a pull request. This feature was recently added to the github web UI: https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ And is in the process of being supported in `hub`, the official command line git augmentation tool for github: mislav/hub#1187 The idea is this is a suggestion, not an enforcement. Everything in the quotes is meant to be a short documentation of guidelines for the commiter to use when opening a pull-request, and should be deleted and filled out with pull-request context specific information. None of the pull-request template is enforced by github and can be deleted if the commiter so chooses.
|
would love this as well!!! |
|
Hey @mislav! It's been a little over two weeks now since this PR was opened (and longer if you include the work it builds on). I'd really like to get this merged; is there anything else I can help you with to make merging this easier? |
|
@mislav Bump |
|
If anyone would like to use this patch in the meantime, I've rebased onto |
|
@jawshooah Thanks, but this is never going to be 2.2-stable, since it's a big feature change, so having it stay based on master is alright. Sorry for the wait, everyone! I should be close to pulling this in. I've just released 2.2.4 yesterday and I want 2.3.0 to be the next big feature release. |
|
Seems reasonable to me, and been fully satisfied using my hack for months now, so take as much time as you need, at least as far as I am concerned. |
|
I'm eagerly awaiting the official release (automated dotfiles installs, Em sex, 12 de ago de 2016 às 18:14, Nick LaMuro notifications@github.com
|
|
@mislav When do you think this will hit a release? Would really like this feature... |
|
@jjeffrey-bolste please see #1232, it's planned for the next big release, and there's also #1187 (comment) from @jawshooah that you can use in the meanwhile :) |
|
I guess what I was asking is when is that release planned for? |
|
I plan to make a prerelease this weekend so people can download a prebuilt binary from the Releases section and try out the new features, including this one! The actual stable release with all the new features will take a few weeks more, because I'm taking a 2-week vacation starting next week, so I won't be around the work on this project and review PRs during this time. |
|
Thank you @jez @NickLaMuro @ganmacs @jeroenvisser101 for all the hard work, review, and discussion! This took so long because it was a tricky feature to implement and review, but now it's all done! I'll try to make a prerelease today so people can download and try a precompiled version with the latest features in master, including this one. |
|
Thanks @mislav, appreciate all the effort you put in hub 💯 |
|
\0/ |
|
🚀 |
|
@mislav did you forget to mention PR templates in the changelog? |
|
@jeroenvisser101 No, it's in there. I call them “repo-specific” templates, for lack of a better term. |
|
Ah, I see! :) |
This pull request includes all the changes from #1145, including changes
made by @ganmacs and @NickLaMuro, with two small additional fixes on top
of those.
The reviewer is encouraged to see the conversation in #1145.
This pull request itself includes changes that
templates
case-insensitivity and file extension. See the GitHub docs for
more information.
Supercedes #1145.
Closes #446, #937, #964, #1118.