Chris, Unfortunately, some PRs contain many commits and even with good commit messages, it's may be hard to figure out what exactly all the commits are trying to accomplish. Therefore, I still think these 2 questions help any committer/merger/reviewer get a quick handle on the intent of the PR:
At a high-level, what does this PR do? >> It creates unicorns when you click on the unicorn button. What is the best way to verify this PR? >> Open TP, click on the unicorn button in the top right corner, and watch the pretty unicorns. I can't tell you how many times I've had to read through commit messages and/or code to try to figure out what a PR is trying to accomplish and how to verify it. I'm just trying to save the reviewer/merger some time.. Jeremy On Wed, Feb 28, 2018 at 1:31 PM, Chris Lemmons <[email protected]> wrote: > `What effect should I expect to see from this change?` > > This is a perfect question, and one that absolutely needs to be > answered. But either > > a) it is answered already in the commit message; or > b) the commit message is insufficient and needs to be `git -amend`ed. > > I definitely wouldn't want a PR that contained that information only > in the PR body, > and there's not a whole lot of value in asking them to re-type it. The > "copy/paste" thing > at the top is already a bit of duplication anyway. > > On Wed, Feb 28, 2018 at 11:03 AM, Dan Kirkwood <[email protected]> wrote: > > followup: rather than > > `What is the best way to verify this PR? ` > > > > what about > > > > `What effect should I expect to see from this change?` > > > > On Wed, Feb 28, 2018 at 10:25 AM, Dan Kirkwood <[email protected]> > wrote: > > > >> +1 on keeping it short and to the point... > >> > >> On Wed, Feb 28, 2018 at 10:14 AM, Jeremy Mitchell < > [email protected]> > >> wrote: > >> > >>> Chris, I really wanted the PR template to be less daunting and super > short > >>> and to the point. It's intention is to give a super quick summary of > >>> what's > >>> included in this PR to help the merger... > >>> > >>> Example: > >>> > >>> #### What does this PR do? Is there a related Github issue? > >>> > >>> "See Issue #1245" or "This PR cascade deletes all delivery service > regexes > >>> when a delivery service is deleted" > >>> > >>> #### What is the best way to verify this PR? <-- IMO this is really > >>> important for the merger so I know how to "test" or "verify" the > >>> functionality. > >>> > >>> Hit the DELETE /api/1.3/deliveryservices/:id endpoint and ensure all > >>> entries in the deliveryservice_regex table are deleted for that > delivery > >>> service. > >>> > >>> #### Does your PR include any of the following? > >>> > >>> - [ ] Tests > >>> - [ ] Documentation > >>> - [X] CHANGELOG.md entry > >>> > >>> ^^ I wasn't trying to imply that those last things were required. I > just > >>> wanted to provide a checklist that might be helpful for the contributer > >>> and > >>> the merger. For example, I always for get to look for a CHANGELOG.md > >>> entry... > >>> > >>> Jeremy > >>> > >>> > >>> On Tue, Feb 27, 2018 at 9:47 PM, Chris Lemmons <[email protected]> > >>> wrote: > >>> > >>> > If there's a relevant GitHub issue, that should be noted in the > >>> > check-in comment, I think. Same for "what does it do?" I don't > usually > >>> > want to spell out steps for someone to verify my stuff because those > >>> > are the steps that I took to verify it. The PR is so you can see the > >>> > things I didn't see. And the commit list will make the presence of > >>> > tests, documentation, and a changelog entry really obvious. > >>> > > >>> > Taking yours and reformatting a bit, what if we did something like > this? > >>> > > >>> > ... > >>> > > >>> > *Describe your PR:* _(copy/paste from changeset comments is > >>> encouraged!)_ > >>> > > >>> > > >>> > > >>> > *Quick Checklist:* > >>> > > >>> > - [ ] Each commit message tells you everything you need to know about > >>> > the change. (Squashing can help with this.) > >>> > - [ ] If applicable, the commit message mentions the appropriate > issue > >>> > number. > >>> > - [ ] This PR does *not* fix a serious security flaw. (Read more: > >>> > www.apache.org/security ) > >>> > - [ ] Automatic code formatters (like gofmt) have been run. > >>> > > >>> > *Tests:* > >>> > > >>> > - [ ] Are not necessary. > >>> > - [ ] Would be helpful, but aren't in this PR. > >>> > - [ ] Are present, but incomplete. > >>> > - [ ] Are included. > >>> > > >>> > *Doc updates:* > >>> > > >>> > - [ ] Are not necessary. > >>> > - [ ] Would be helpful, but aren't in this PR. > >>> > - [ ] Are present, but incomplete. > >>> > - [ ] Are included. > >>> > > >>> > *If there's no update to CHANGELOG.md, why not?* > >>> > > >>> > *Does this break backward compatibility?* > >>> > > >>> > *Is there anyone specific that ought to take a look at this?* > >>> > > >>> > ... > >>> > > >>> > We want to be friendly to committers, while still getting good > >>> > information for checking PRs. I could be easily convinced that the > >>> > "Tests" or "Doc updates" sections in there are too long, but I think > >>> > it should be clear that a potential committer can offer up some code > >>> > without hitting 100% on tests, docs, and such. > >>> > > >>> > On Tue, Feb 27, 2018 at 1:24 PM, Jeremy Mitchell < > [email protected] > >>> > > >>> > wrote: > >>> > > How about something like this for a PR template? > >>> > > > >>> > > #### What does this PR do? Is there a relevant Github issue? > >>> > > > >>> > > > >>> > > #### What is the best way to verify this PR? > >>> > > > >>> > > > >>> > > #### Does your PR include any of the following? > >>> > > > >>> > > - [ ] Tests > >>> > > - [ ] Documentation > >>> > > - [ ] CHANGELOG.md entry > >>> > > > >>> > > On Tue, Feb 27, 2018 at 10:46 AM, Jeremy Mitchell < > >>> [email protected] > >>> > > > >>> > > wrote: > >>> > > > >>> > >> With an issue and/or pr template, we will have 6/6 items checked: > >>> > >> > >>> > >> https://github.com/apache/incubator-trafficcontrol/community > >>> > >> > >>> > >> I actually think PR templates would be quite helpful. As a > >>> > >> committer/merger, it would be nice to know what problem the PR is > >>> > solving > >>> > >> and how to verify the functionality. A PR template could also help > >>> > >> contributors ensure that their PRs are complete. I.e. does this PR > >>> > includes > >>> > >> tests, documentation, etc. > >>> > >> > >>> > >> I'll take a stab at a couple of templates and run them by the > group. > >>> > >> > >>> > >> Jeremy > >>> > >> > >>> > >> On Wed, Jan 31, 2018 at 1:10 PM, Chris Lemmons < > [email protected]> > >>> > wrote: > >>> > >> > >>> > >>> I'm +1 on Issue Templates, for sure. I don't know that PR > templates > >>> > >>> are quite as critical, but it might be nice to have a reminder in > >>> > >>> there about verifying that the changelog is updated if necessary > and > >>> > >>> documentation for new features is present. If the PR Template > >>> > >>> overwrites the default comment that you get from the commit > body, it > >>> > >>> might be more annoying than valuable, though. > >>> > >>> > >>> > >>> I'm also +1 on hiding these particular files in a .github > directory. > >>> > >>> Unlike CONTRIBUTING and README, they don't provide all that much > >>> > >>> benefit for a new person looking for stuff to read. > >>> > >>> > >>> > >>> On Wed, Jan 31, 2018 at 12:17 PM, Durfey, Ryan < > >>> > [email protected]> > >>> > >>> wrote: > >>> > >>> > Always +1 on standardization and consistency > >>> > >>> > > >>> > >>> > I still want to circle back and setup project/kanbans for > >>> organizing > >>> > >>> tickets in Github. > >>> > >>> > > >>> > >>> > Ryan Durfey M | 303-524-5099 > >>> > >>> > CDN Support (24x7): 866-405-2993 or [email protected] > >>> <mailto: > >>> > >>> [email protected]> > >>> > >>> > > >>> > >>> > From: Dewayne Richardson <[email protected]> > >>> > >>> > Reply-To: "[email protected]" < > >>> > >>> [email protected]> > >>> > >>> > Date: Wednesday, January 31, 2018 at 11:15 AM > >>> > >>> > To: "[email protected]" < > >>> > >>> [email protected]> > >>> > >>> > Subject: Github PR/Issues Format Templates > >>> > >>> > > >>> > >>> > I was working through the go-swagger repo and found a bug. I > >>> > submitted > >>> > >>> a > >>> > >>> > new issue and found this interesting approach I think the TC > >>> github > >>> > >>> should > >>> > >>> > adopt, "Issue and PR Templates". I think the main value here > is > >>> > >>> > consistency in our PRs/Issues and user friendly prompts to say > >>> "these > >>> > >>> are > >>> > >>> > the data points we need to help you solve your issue". > >>> > >>> > > >>> > >>> > Working example: > >>> > >>> > https://github.com/go-swagger/go-swagger/issues/new > >>> > >>> > > >>> > >>> > Github Doc on how to implement templates: > >>> > >>> > https://github.com/blog/2111-issue-and-pull-request-templates > >>> > >>> > > >>> > >>> > If we think it's a good idea, then I'll respond with some > examples > >>> > for > >>> > >>> > Issues and PR's that we can discuss. > >>> > >>> > > >>> > >>> > -Dew > >>> > >>> > > >>> > >>> > >>> > >> > >>> > >> > >>> > > >>> > >> > >> >
