Zeroclickinfo-goodies: Pull Request template (a review)

[GuiltyDolphin]

@duckduckgo/community-leaders Right, we've had the PR template a while now, just thought I'd ask how you guys have found it (both as reviewers and users).

My only complaint is the checklist, which feels a bit cumbersome (though mostly people are using it correctly). Provided everyone uses good titles for their pull requests, I think the checklist becomes redundant.

Also, as @swapagarwal pointed out, the checklist can actually obscure any PR-specific checklists.

Any thoughts on that? Anything else?

@duckduckgo/duckduckhack-contributors feel free to chime in if you have any feedback/suggestions!

[mintsoft]

I think it feels quite .. noisy? When I do a PR the first thing I do is delete most of it; I guess in the ideal world when people hit the "PR" button then they'd be given a set of instructions of what to do but the actual box to be filled in would be empty!

[GuiltyDolphin]

@mintsoft Yeah - the fields I think are _required_ are the description (we don't want empty PRs), and the IA page/maintainer. We could probably expand the description to suggest they should mention any issues it fixes there - and @mentions, thoughts?

[gaulrobe]

I concur that the checkboxes are a bit noisy. The rest of the PR template is structured in a way that makes sense and is especially helpful in trying to get details about a particular PR... but it is only helpful if people remember to fill it in! That problem is a hard one to fix. 😄

I see the top checkbox part more of a guide on what the expectations of the PR _should_ be (title, what kind of change is it, etc). Just changing to from a bunch of checkboxes to some bulleted guidelines might be worthwhile?

[GuiltyDolphin]

@gaulrobe Hmm.. Maybe, like a very short summary of what needs to be done before submitting? Perhaps:

Before submitting your pull request, please make sure you have done the following:

• Made sure there are no existing pull requests for the changes.
• Given an appropriate title to the pull request, e.g., Calculator: Add factorial function.
• Linked the relevant Instant Answer page and maintainer.

That _sort_ of thing? (This is basically CONTRIBUTING - but we don't seem to get much readthrough of that).

[gaulrobe]

@GuiltyDolphin yeah, just some boilerplate like that. Having expectations written down is always helpful, some people will see it and follow the directions, others won't... but at least there won't be anything that inhibits those that leverage the description more heavily.

[GuiltyDolphin]

@zachthompson @moollaza Any thoughts? Are you happy with the idea of a short, bulleted list rather than the check boxes?

[moollaza]

Yes, I'd prefer bullets instead of check boxes

Or simply headings + description text.

The template should only aim to clarify the PR, we shouldn't add in other instructions or tell them how to make a PR. As you said, the CONTRIBUTING does that as well as the Docs. First timer's also get a Dax message that can explain things.

This template should just provide the framework for a well formatted PR description so that devs understand what questions we need them to answer when they make a PR. That way its optimized for people who will continuously see the PR template. Right now it feels like the first step is deleting most of the template text, which is too cumbersome

[swapagarwal]

How about something like this? This gives the best of both worlds: no more task list + no more deleting irrelevant text! Just uncomment the relevant item and only that will be rendered.

###### What does your Pull Request do?

Uncomment the most relevant item and use the following title template to name
your Pull Request.

[//]: # "(New Instant Answer) Cheat Sheets: **`New {Cheat Sheet Name} Cheat Sheet`**"
[//]: # "(New Instant Answer) Other: **`New {IA Name} Instant Answer`**"
[//]: # "(Improvement) Bug fix: **`{IA Name}: Fix {Issue number or one-line description}`**"
[//]: # "(Improvement) Enhancement: **`{IA Name}: {Description of Improvements}`**"
[//]: # "(Non–Instant Answer) Other (Role, Template, Test, Documentation, etc.): **`{GoodieRole/Templates/Tests/Docs}: {Short Description}`**"

###### Description of changes

Provide an overview of the changes this pull request introduces.
...

This renders as:

What does your Pull Request do?

Uncomment the most relevant item and use the following title template to name
your Pull Request.

Description of changes

Provide an overview of the changes this pull request introduces.
...

[GuiltyDolphin]

@swapagarwal Huh, didn't know GHFM supported comments (it doesn't support <> anyway); awesome.

Yeah, @swapagarwal I think something along those lines is probably best; I think a mixture of short bullets in comments would be good (then we don't get the extra visuals we don't need).

[moollaza]

Huh, didn't know GHFM supported comments (it doesn't support <> anyway); awesome.

They _don't_ support comments :) -- This is just confusing/abusing the parser: http://stackoverflow.com/a/20885980/819937

They do support HTML comments which would also work: ie <!-- comment here -->

[GuiltyDolphin]

@moollaza Hehe, I read that exact post - not sure how I missed the bit about it being a bit abusive though :wink:

[pjhampton]

I'll close this for now. https://github.com/duckduckgo/zeroclickinfo-goodies/pull/3983 was merged earlier. I guess the approach is to alter as the direction changes. Feel free to make unsolicited changes 👍 😄