Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-20965: Add PR template #10758

Merged
merged 1 commit into from
Jul 28, 2017
Merged

CRM-20965: Add PR template #10758

merged 1 commit into from
Jul 28, 2017

Conversation

mickadoo
Copy link
Contributor

@mickadoo mickadoo commented Jul 25, 2017

Overview

Github offers pull request templates to standardize the content of pull requests. This should help improve the description of pull requests to core.

Before

There is no pull request template, or structure.

After

A new template is added which will be automatically added to the description input whenever a new pull request is created.

Comments

  • This pull request description uses the new template.
  • This template is modified from the CiviHR version
  • Please feel free to suggest a better format for the template

@totten
Copy link
Member

totten commented Jul 25, 2017

  • Big +1 on adding a template for PR descriptions.
  • I really like the Overview/Before/After and the links for screenshot tools.
  • Tempted to drop "Technical Details". That's just a gut reaction -- don't have a pithy rationalization.
  • Tempted to add "Testing", "Review Suggestions", or "Risks". Basically, as a reviewer, I appreciate when authors clearly communicate about (a) what scenario/configuration they tested and (b) what remaining risks or test-activities merit attention or disclaimers. (Ideally, the reviewers could help with those; or maybe pull-in someone else; or maybe publish a note with RC.)

These are examples of what I mean:

  • "This code uses some edgy HTML5 stuff -- I tested in Chrome/FF/Safari but don't have IE8, but it would be great if someone could test in IE8."
  • "I tested this with the PayPal.com sandbox, but the codepath is also used by Authorize.net. Someone with Authorize.net access should try it out."
  • "These options X, Y, Z can be mixed in many ways. I've spot-checked X1+Y{2,3}+Z{2,5} but don't have access/capacity to test X2 and Z3."

@mickadoo
Copy link
Contributor Author

@totten

I'm fine with dropping technical details, for CiviHR it's used as an overview for bigger PRs to introduce a new service or architecture change. If it's not likely to be used for the majority of PR descriptions then I'll drop it, just let me know.

For CiviHR for testing we just have a "tests pass" checkbox, but with Jenkins this isn't required here (and hopefully won't be in the future when we get Jenkins set up with CiviHR). I'm happy to add another section like "Suggestions" as long as you think it will be used by most descriptions - the key is to keep it minimal and allow expansion where the developer feels the need.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

So the status of this PR as I understand it is that it is agreed it is an improvement. There are some soft suggestions that are not resolved but are not blockers to merging. It feels no-risk & easy to edit later as people start to see the potential - we should merge this now I think & fix more later if we wish

@mickadoo
Copy link
Contributor Author

@eileenmcnaughton that's a fair assessment. I was waiting a bit to see what the consensus is on dropping "technical details" or adding new sections. What do you think yourself?

@eileenmcnaughton
Copy link
Contributor

Honestly I don't know - I think once we are using it suggestions will come in thick & fast - merging!

@eileenmcnaughton eileenmcnaughton merged commit 34e17e1 into civicrm:master Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants