Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

feat(pr-template): initial work on template #149

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

mgan59
Copy link
Contributor

@mgan59 mgan59 commented Jun 15, 2020

Purpose

Adding our initial PR Template so that we can improve our process for managing change-requests. Refers to issue #122

What changes were made?

Created the .githhub/ folder and generated the corresponding PULL_REQUEST_TEMPLATE.md file and placed the template into the folder.

Verification

To view the RAW Version of the Template

The Default Look When PR Opened/Drafting

Quick Visual of the default look of the PR Template inside of the PR Panel when opened/draft-mode.

2020-06-15 08 15 51

What the Process Looks Like to Fill-Out

Quick Visual of what it looks like to fill it out, once merged it will auto-populate.

2020-06-15 08 07 41


Checklist

Pull-request reviewer should ensure the following

  • Are issues linked correctly?
  • Is this PR labeled correctly?
  • If template updates: do they align with developers.google.com/style/?
  • Did the PR receive at least one 👍 and no 👎 from core-maintainers?
  • On merging, did you complete the merge using keywords?

@mgan59 mgan59 added the improve For any suggestions about how to make templates better. label Jun 15, 2020
@mgan59 mgan59 linked an issue Jun 15, 2020 that may be closed by this pull request

---

## Type of Change
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debating if Type of Change should sit higher up in the Template, it is nice to know what exactly this change-type is before really digging into a PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd err toward suggesting we don't include Type of Change, at least not in our first iteration. I'm not sure how close these categories will align with future work we do. And I wonder how much value it adds, verses the confusion it introduces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding this. It seems to me that categorizing is what labels are for and then it's more of a triage task for reviewers. Might be better suited as a checklist item, below (e.g., "Is this PR labeled correctly?")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can hold off and I'll remove the section. Labels will work for now.

@mgan59 mgan59 mentioned this pull request Jun 15, 2020
@mgan59
Copy link
Contributor Author

mgan59 commented Jun 15, 2020

We could re-use the animated gif, specifically the second one, and embed it into our Contributor.md file so that there is an animation showing the expectations of filling out the PR Template.

> __Pull-Request Reviewer__ should ensure the following

* [ ] Are Issues Linked Correctly?
* [ ] If, template updates do they align with [Google StyleGuide]()?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to link to the StyleGuide? If so which link?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, great to link to the Style Guide. Link to https://developers.google.com/style/


* [ ] Are Issues Linked Correctly?
* [ ] If, template updates do they align with [Google StyleGuide]()?
* [ ] Did the PR receive :+1: from core-maintainers?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check to see what roles we define for our project. Right now, the only official role we have is a PSC member. (This will likely be extended soon.)

I think we need to quantify support from core maintainers:
[ ] Did the PR receive at least one 👍 and no 👎 from core-maintainers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making your line suggestion, we can always update this for new roles.

@camerons
Copy link
Member

I love your work @mgan59 . Let's discuss in our meeting and then roll out.

Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot, @mgan59! As someone who hasn't contributed a PR yet, I like how this gives me a really clear set of things to work through to make sure my PR is ready for review.

My only substantive suggestion is replacing the Type of Change section with a checklist item (see line comment). I do have some nit-picky style suggestions to make, though (the big thing: sentence case instead of title case) after high-level issues are resolved (or I can leave a bunch of line comments immediately, if you'd prefer).

Thank you!


---

## Type of Change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding this. It seems to me that categorizing is what labels are for and then it's more of a triage task for reviewers. Might be better suited as a checklist item, below (e.g., "Is this PR labeled correctly?")

@mgan59
Copy link
Contributor Author

mgan59 commented Jun 17, 2020

@ddbeck Yeah saw the title case as I was editing, fixed that. Pushing updates.

Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Morgan, we talked about this briefly in the meeting today. Since there hasn't been any other comments on the substance of this PR, this is nearly ready.

I'm suggesting a few very minor changes for capitalization, spelling, etc. If there are any questions about this, let me know, but it shouldn't be too exciting.

Once those comments are taken care of, we can get this merged! Thank you!

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
@camerons
Copy link
Member

@Loquacity Lana, in one of our conversations you were suggesting git templates. You might want to review Morgan's proposal here.

Copy link
Member

@Loquacity Loquacity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks really good. Nice work!

@mgan59
Copy link
Contributor Author

mgan59 commented Jun 28, 2020

Thanks for the updates/suggestions @ddbeck made all of the changes. 😄

@mgan59
Copy link
Contributor Author

mgan59 commented Jun 28, 2020

Viewable - Final Preview

@ddbeck
Copy link
Contributor

ddbeck commented Jun 29, 2020

This looks good to me. Great work, @mgan59!

@camerons Based on our conversation in the last meeting, I think this is ready to be merged. Would you do the honors? (I'm not a committer on the repo)

@emckean emckean merged commit e36d70e into master Jul 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improve For any suggestions about how to make templates better.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR Template Initial
5 participants