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

Add .github/ templates #172

Merged
merged 2 commits into from
Jun 6, 2016
Merged

Add .github/ templates #172

merged 2 commits into from
Jun 6, 2016

Conversation

daenney
Copy link
Member

@daenney daenney commented Jun 5, 2016

This moves CONTRIBUTING.md to .github/ and adds templates for when someone opens an issue or a PR.

The wording in all of this is obviously up for discussion, so, discuss!

That thing gets interpreted as Markdown and rendered as an h1.
@daenney
Copy link
Member Author

daenney commented Jun 5, 2016

@voxpupuli/collaborators Sorry for the wide reach but this is important for all our projects!

@@ -93,5 +93,3 @@ If you don't want to have to recreate the virtual machine every time you
can use `BEAKER_DESTROY=no` and `BEAKER_PROVISION=no`. On the first run you will
at least need `BEAKER_PROVISION` set to yes (the default). The Vagrantfile
for the created virtual machines will be in `.vagrant/beaker_vagrant_fies`.

# vim: syntax=markdown
Copy link
Member Author

@daenney daenney Jun 5, 2016

Choose a reason for hiding this comment

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

This modeline is valid markdown and hence gets rendered as an h1. If you're having problems with getting .md files to render correctly in vim please add this to your .vimrc instead:

autocmd BufNewFile,BufReadPost *.md set filetype=markdown

@daenney daenney force-pushed the templates branch 3 times, most recently from 10a3a05 to 042215c Compare June 5, 2016 13:57
- [ ] There is no existing PR that addresses this problem
- [ ] Mentioned any existing issues in your commit so they get linked and
closed once this PR gets merged, i.e: `Closes #1554` in te body of a commit
- [ ] Followed the instructions in the [Contributing](https://github.com/Homebrew/homebrew-core/blob/master/.github/CONTRIBUTING.md) document
Copy link
Member Author

Choose a reason for hiding this comment

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

So obviously this shouldn't point to Homebrew, that's just a link copy-paste, but how do we generate this per repository?

@daenney daenney force-pushed the templates branch 4 times, most recently from 5cb4cd6 to 99706a0 Compare June 5, 2016 14:11
@liamjbennett
Copy link
Member

I'm pleased we are adding this. No concerns from me. Happy to role this out.

- [ ] Ran the unit/spec tests and ensured they still pass
- [ ] Added tests to cover the new behaviour
- [ ] Updated the documentation to match the changes
- [ ] Squashed your PR down to a single commit
Copy link
Member

Choose a reason for hiding this comment

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

Hi highly disagree. A commit should be a single logical change. But a PR can have multiple changes. We once had a detailed explanation about the commits in the CONTRIBUTING.md but that got removed (does anyone know why?)

Copy link
Member Author

@daenney daenney Jun 5, 2016

Choose a reason for hiding this comment

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

Hence why there is this, at the top of the file:

+If this PR tries to address multiple issues please split the PR out in a new
+PR per feature/issue, unless that's not possible because of how the code builds
+on top of each other. In that case ensure every feature/fix and its associated
+tests and documentation are squashed into a distinct commit.

I believe that covers your concern. I'm happy to move the text around.

@bastelfreak
Copy link
Member

Should we add a note to PULL_REQUEST_TEMPLATE.md that somebody who creates the PR already updates the CHANGELOG.md?

@bastelfreak
Copy link
Member

here is the old version of our CONTRIBUTING with the git guidelines https://github.com/virtapi/installimage/blob/master/CONTRIBUTING.md

@petems
Copy link
Member

petems commented Jun 5, 2016

LGTM! 👍


- [ ] There is no existing PR that addresses this problem
- [ ] Mentioned any existing issues in your commit so they get linked and
closed once this PR gets merged, i.e: `Closes #1554` in te body of a commit
Copy link
Member

Choose a reason for hiding this comment

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

there's a typo here, s/te/the

@daenney
Copy link
Member Author

daenney commented Jun 5, 2016

@bastelfreak Better this way?

@daenney daenney force-pushed the templates branch 2 times, most recently from 8a5dbe8 to e9b8a04 Compare June 5, 2016 19:25
@ferventcoder
Copy link

@daenney take a look at how Chocolatey does the templates - I recommend surrounding this with HTML comment tags so only what you want shows up - I find folks don't actually read the last line that says DELETE this - see https://github.com/chocolatey/choco/issues/new (and look at the preview without changing anything) to get a sense of what I mean and then take a look at the templates at https://github.com/chocolatey/choco/tree/master/.github

@bastelfreak
Copy link
Member

@daenney looks good 👍

What do you think about adding the git notes back to CONTRIBUTION.md?

@daenney
Copy link
Member Author

daenney commented Jun 6, 2016

@ferventcoder Oh I like the trick with the HTML comment tag for that section, I'll do that, thanks!

@petems
Copy link
Member

petems commented Jun 6, 2016

@ferventcoder That is a cool idea! 👍

@ferventcoder
Copy link

ferventcoder commented Jun 6, 2016

I'd take credit for that, but it was @dahlbyk's idea - and a fantastic one at that!

@daenney
Copy link
Member Author

daenney commented Jun 6, 2016

There, I changed the templates around to have less "please remove this" and more "please provide this" instead. The only thing that has a comment block is the PR template

@daenney
Copy link
Member Author

daenney commented Jun 6, 2016

I'm going to go ahead and merge this so we'll have an initial version of this going out the next time people module sync. We can continue to iterate on it through separate PRs.

@daenney daenney merged commit e15047d into master Jun 6, 2016
@daenney daenney deleted the templates branch June 6, 2016 17:58
@dahlbyk
Copy link

dahlbyk commented Jun 6, 2016

I'd take credit for that, but it was @dahlbyk's idea - and a fantastic one at that!

Glad to hear it's working well. 😀

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.

7 participants