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 a Contributing Guidelines file #314

Merged
merged 1 commit into from
Mar 2, 2017
Merged

Conversation

gdams
Copy link
Member

@gdams gdams commented Jan 9, 2017

We should have a contributing guidelines file:

Inital Draft, open to ideas

@gdams
Copy link
Member Author

gdams commented Jan 9, 2017

#313

gibfahn

This comment was marked as off-topic.

@gdams gdams force-pushed the contributing branch 2 times, most recently from f5d4a0a to 650d998 Compare January 10, 2017 13:32
gdams

This comment was marked as off-topic.

@gdams gdams force-pushed the contributing branch 3 times, most recently from b9708b8 to 568f7bb Compare January 16, 2017 16:49
gibfahn

This comment was marked as off-topic.

MylesBorins

This comment was marked as off-topic.

@gdams
Copy link
Member Author

gdams commented Jan 26, 2017

@MylesBorins @gibfahn Updated PTAL

gibfahn

This comment was marked as off-topic.

gibfahn

This comment was marked as off-topic.

gibfahn added a commit to gibfahn/citgm that referenced this pull request Feb 16, 2017
Using ./CONTRIBUTING.md in the PR template resolves to
https://github.com/nodejs/citgm/compare/CONTRIBUTING.md. Use the full path
instead (as is done in Node.js core).

Note that the CONTRIBUTING.md file hasn't yet been added (the PR to add it is
nodejs#314).
gibfahn added a commit to gibfahn/citgm that referenced this pull request Feb 16, 2017
Using ./CONTRIBUTING.md in the PR template resolves to
https://github.com/nodejs/citgm/compare/CONTRIBUTING.md. Use the full path
instead (as is done in Node.js core).

Note that the CONTRIBUTING.md file hasn't yet been added (the PR to add it is
nodejs#314).
@gibfahn
Copy link
Member

gibfahn commented Feb 16, 2017

Also can you change the commit subsystem from docs: to doc:?

@gibfahn
Copy link
Member

gibfahn commented Feb 16, 2017

Incidentally, given that node-review works in citgm, it's pretty straightforward to include the PR-URL: and Reviewed-By: labels in PRs. I think including PR-URL: is definitely worth it for landing other people's commits.

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 16, 2017

@gibfahn we originally decided not to include all the meta data to make it a lower barrier to entry for contributing. I'm open to introducing it if @nodejs/citgm is on board

@MylesBorins
Copy link
Contributor

@gibfahn one thing to mention, node-review will only count LGTM's from people who are a core collaborator, since it is hardcoded. The fact that it "mostly" works is a happy accident

gibfahn added a commit to gibfahn/citgm that referenced this pull request Feb 20, 2017
Using ./CONTRIBUTING.md in the PR template resolves to
https://github.com/nodejs/citgm/compare/CONTRIBUTING.md. Use the full path
instead (as is done in Node.js core).

Note that the CONTRIBUTING.md file hasn't yet been added (the PR to add it is
nodejs#314).
@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

@MylesBorins good points. Okay, so how about optionally using PR-URL: and not bothering with the rest?

@MylesBorins
Copy link
Contributor

Sgtm

MylesBorins pushed a commit that referenced this pull request Feb 21, 2017
Using ./CONTRIBUTING.md in the PR template resolves to
https://github.com/nodejs/citgm/compare/CONTRIBUTING.md. Use the full path
instead (as is done in Node.js core).

Note that the CONTRIBUTING.md file hasn't yet been added (the PR to add it is
#314).
@gdams
Copy link
Member Author

gdams commented Feb 27, 2017

SGTM

@gdams gdams force-pushed the contributing branch 2 times, most recently from d90ada7 to 5c7661c Compare February 27, 2017 11:26
@gdams
Copy link
Member Author

gdams commented Feb 27, 2017

@nodejs/citgm I think this is ready, could I get some reviews please?

@gdams gdams force-pushed the contributing branch 5 times, most recently from 1f4fd0e to 3dff18e Compare February 27, 2017 11:40
gibfahn

This comment was marked as off-topic.

@MylesBorins
Copy link
Contributor

I feel like the commit guidelines are overkill.if we are not generating a changelog do we really need anything more than a descriptive title?

@gibfahn
Copy link
Member

gibfahn commented Feb 28, 2017

I feel like the commit guidelines are overkill.

Which ones exactly? The 50 (imperative verb), leave a line, 72 is just the standard Git commit guidelines AFAIK. Subsystem we've been doing already.

Other than that are there any other rules?

EDIT: If you're saying that commits only need to be one line long, then I don't think that's forbidden by the current description.

@targos
Copy link
Member

targos commented Feb 28, 2017

I think it's good to have some guidelines, even if we do not generate a change log.

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 28, 2017 via email

@sxa
Copy link
Member

sxa commented Feb 28, 2017

At the moment when a PR is created it points you to the guidelines at https://github.com/nodejs/citgm/blob/master/CONTRIBUTING.md - can I suggest removing that until this has landed?

@gdams
Copy link
Member Author

gdams commented Feb 28, 2017

@sxa555 this is about to be landed, no need to worry

@gdams
Copy link
Member Author

gdams commented Mar 1, 2017

@nodejs/citgm are we all happy with this now ?

bzoz

This comment was marked as off-topic.

targos

This comment was marked as off-topic.

@gdams
Copy link
Member Author

gdams commented Mar 2, 2017

@MylesBorins that leaves you then?

MylesBorins

This comment was marked as off-topic.

@gdams gdams merged commit 05fd3ed into nodejs:master Mar 2, 2017
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.

6 participants