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

Feature/pr template #15

Merged
merged 4 commits into from
Apr 11, 2018
Merged

Feature/pr template #15

merged 4 commits into from
Apr 11, 2018

Conversation

tstone
Copy link
Collaborator

@tstone tstone commented Apr 3, 2018

In talking with @lucas-ibotta about this repo, it seems like it would be helpful to call out a few of the necessary tasks when submitting a PR. This PR takes a first pass at a gem-specific PR template.

* [ ] Specs written and passing
* [ ] Backwards-incompatible changes called out in this PR
* [ ] Increment the version file (`./lib/atomic_cache/version.rb`) when applicable
* See [semver.org](https://semver.org/) for what segment to increment
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are lines 14 and 15 enough? I'm a little concerned about the "when applicable" phrasing as it might not be obvious to everyone when/if this should happen.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/Ibotta/sopstool/blob/master/.github/PULL_REQUEST_TEMPLATE.md is slightly different - it has a separate versioning section

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'm OK with this. I think that if someone's not sure they'll ask.

@tstone tstone mentioned this pull request Apr 3, 2018
Copy link
Contributor

@blimmer blimmer left a comment

Choose a reason for hiding this comment

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

Once we land here, we should add this to our internal open-source generator.


@mention any specific individuals you'd like to be aware of this change:
* Who is impacted by the work?
* Who knows this code best and can provide useful feedback?
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this section is really valuable for our internal PR template, but does this provide value to external contributors? I worry that many might feel left-out before of this, thinking "I don't know, but I want to contribute!".

Maybe we omit the whole cc section?

Choose a reason for hiding this comment

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

That seems good to me


@mention any specific individuals you'd like to be aware of this change:
* Who is impacted by the work?
* Who knows this code best and can provide useful feedback?
Copy link
Contributor

Choose a reason for hiding this comment

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

Related, should we add a CODEOWNERS file in this repo?

https://help.github.com/articles/about-codeowners/

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a poll on this in the #ibotta_open_source slack channel.

* [ ] Specs written and passing
* [ ] Backwards-incompatible changes called out in this PR
* [ ] Increment the version file (`./lib/atomic_cache/version.rb`) when applicable
* See [semver.org](https://semver.org/) for what segment to increment
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'm OK with this. I think that if someone's not sure they'll ask.

Copy link

@pizza-planet pizza-planet left a comment

Choose a reason for hiding this comment

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

Agree with Ben about adding this to the generator and omitting CC section. But with those changes, it LGTM.


@mention any specific individuals you'd like to be aware of this change:
* Who is impacted by the work?
* Who knows this code best and can provide useful feedback?

Choose a reason for hiding this comment

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

That seems good to me

@tstone tstone merged commit cd56664 into master Apr 11, 2018
@tstone tstone deleted the feature/pr-template branch April 11, 2018 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants