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

Improve Pull Request template #374

Merged
merged 3 commits into from
Oct 23, 2017
Merged

Improve Pull Request template #374

merged 3 commits into from
Oct 23, 2017

Conversation

agisilaos
Copy link
Contributor

@agisilaos agisilaos commented Oct 19, 2017

↖ First, change the base branch from "master" to "dev".

↑ Next, briefly describe your proposal in the title.

Finally, tell us more about the change here, in the description.

Hey, everyone, this is my first contribution to primer-css and I would love to know what you think. So I was thinking of adding a new line into the PR Template: Fixes: #

That way when someone creates a Pull request can mention the issue that has been fixed more easily.

Benefits

  • Conversation can be more focused on the issue when sometimes the title of the PR is not so descriptive (It happens we're humans)
  • When a PR is merged the issue can be automatically closed so we don't end up with situations like this: Add docs folder back to utilities package and remove docs from gitignore #305. This issue has been fixed and the PR related to the issue is merged but the issue is not closed. In addition to that, we will not have to think all the time to close an issue manually when we fix it with a PR because it will happen automatically.

I don't know if my logic here is correct so I would love to know what you think about this potential change.

P.s Love how you build primer-css in the open. Learned a lot of things about how to communicate more effectively through issues and PR's, write better commit messages and organize my open source projects.

/cc @primer/ds-core

@agisilaos agisilaos changed the title Improve Pull request template Improve Pull Request template Oct 19, 2017
@shawnbot
Copy link
Contributor

Thanks for figuring out the branch issue on this, @agisilaos, and for reopening. This is a good change, but I'd like to add a bit more language around it so that it matches the tone of the first two instructions ("First, ...", "Next, ..."). Something like this, maybe?

↖ First, change the base branch from "master" to "dev".

↑ Next, briefly describe your proposal in the title.

If this fixes or otherwise closes an issue, include this line with the issue number after the #:
Fixes: #

...

I also couldn't help but notice that you didn't delete the instructions. I'm curious: Is that a convention that you're unfamiliar with, and if so should we be more explicit about asking folks to delete each line as they check it off the list?

Thanks again!

@agisilaos
Copy link
Contributor Author

agisilaos commented Oct 19, 2017

I really like your proposal @shawnbot! Makes total sense. I will add it with my next commit.

Indeed it's a convention that I'm unfamiliar with. I think It will make more sense if we create a checklist for these 3 steps like it's presented below:

  • First, change the base branch from "master" to "dev".

  • Next, briefly describe your proposal in the title.

  • If this fixes or otherwise closes an issue, include this line with the issue number after the #:
    Fixes: #

Also, I have another idea to go your suggestion regarding this message even further:

Proposal for the message:
If this fixes or otherwise closes an issue, include this line with the issue number after the #. If not add a - after the #

Last minute thoughts:
By adding a checklist contributors will not have to remove lines from the template so it will be a bit more natural as a flow for them. What do you think?

@shawnbot
Copy link
Contributor

That's a great idea! Lerna does this in its pull request template, and it was pretty nice to use.

I could see just having a checklist item for whether it fixes any issues, e.g. something like:

- [ ] Fixes # (type an issue number after the # if applicable)

Also fixed the naming of the `Fixes: #` with the help of @shawnbot.
@agisilaos
Copy link
Contributor Author

Perfect! I'm so happy with the changes 💯

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contributions. ⭐️✨⚡️🌈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants