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 CONTRIBUTING.md and COLLABORATOR_GUIDE.md #1981

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 96 additions & 0 deletions COLLABORATOR_GUIDE.md
@@ -0,0 +1,96 @@
# jade Collaborator Guide
Copy link
Member

Choose a reason for hiding this comment

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

Can it be in CONTRIBUTING.md as well? It's not a great idea to have a bunch of UPPERCASE files in the root of the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm indifferent. I assume io.js did this to keep the files shorter.

Copy link
Member

Choose a reason for hiding this comment

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

Well there is a talk in io.js about moving those files into separate directory (or a separate repository even). Relevant discussion is here: nodejs/node#12

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should move the collaborator guide somewhere else entirely (perhaps a page on the website) and then just point new collaborators at this guide, since it's not relevant to most contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer having it in the repo because it increases its visibility. For example, when other devs evaluate using jade, they can see our process - and hopefully they like what they see ;)
We could always extract it into static site at a later point in time.
@rlidwka would you be ok with leaving it in a separate file for now?

Copy link
Member

Choose a reason for hiding this comment

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

would you be ok with leaving it in a separate file for now?

yes, it doesn't matter that much


**Contents**

* Issues and Pull Requests
* Accepting Modifications
- Involving the TC
* Landing Pull Requests
- Technical HOWTO

This document contains information for Collaborators of the jade
project regarding maintaining the code, documentation and issues.

Collaborators should be familiar with the guidelines for new
contributors in [CONTRIBUTING.md](./CONTRIBUTING.md).

## Issues and Pull Requests

Courtesy should always be shown to individuals submitting issues and
pull requests to the jade project.

Collaborators should feel free to take full responsibility for
managing issues and pull requests they feel qualified to handle, as
long as this is done while being mindful of these guidelines and the
opinions of other Collaborators.

Collaborators may **close** any issue or pull request they believe is
not relevant for the future of the jade project. Where this is
unclear, the issue should be left open for several days to allow for
additional discussion. Where this does not yield input from jade
Collaborators or additional evidence that the issue has relevance, the
issue may be closed. Remember that issues can always be re-opened if
necessary.

## Accepting Modifications
Copy link
Member

Choose a reason for hiding this comment

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

This chapter seems to be missing merge procedure. Shall we use "The Big Green Button" to merge or wget + git am + git push routine?

Copy link
Member

Choose a reason for hiding this comment

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

I've been using the big green button up until now, but there is a decent argument that we should switch to always rebase. Perhaps we could make the switch after the 2.0.0 release?

Copy link
Member

Choose a reason for hiding this comment

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

What is the argument for rebasing?

Copy link
Member

Choose a reason for hiding this comment

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

A much cleaner commit log that reflects the order in which things were actually added to master, rather than the order in which they were written. I wish GitHub had a "Big Green Rebase In" button (maybe one day I'll build a browser extension to add one).

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I make this choice depending on the number of commits.
If a project gets commits on a daily or even weekly basis and you add a PR that spans multiple commits over multiple weeks, your history looks messy - so rebasing saves the day.
But for jade, PRs are often a small number of commits on the same day, so big green button works just fine.
My vote would be that complex PRs that were kept open for quite a while or have multiple commits over several days or weeks "should" be rebased (meaning it's recommended, not mandatory) - everything else is OK to be just merged.


All modifications to the jade code and documentation should be
performed via GitHub pull requests, including modifications by
Collaborators.

All pull requests must be reviewed and accepted by a Collaborator with
sufficient expertise who is able to take full responsibility for the
change. In the case of pull requests proposed by an existing
Collaborator, an additional Collaborator is required for sign-off.

In some cases, it may be necessary to summon a qualified Collaborator
to a pull request for review by @-mention.

If you are unsure about the modification and are not prepared to take
full responsibility for the change, defer to another Collaborator.

Before landing pull requests, sufficient time should be left for input
from other Collaborators. Leave at least 48 hours during the week and
72 hours over weekends to account for international time differences
and work schedules. Trivial changes (e.g. those which fix minor bugs
or improve performance without affecting API or causing other
wide-reaching impact) may be landed after a shorter delay.

Where there is no disagreement amongst Collaborators, a pull request
may be landed given appropriate review. Where there is discussion
amongst Collaborators, consensus should be sought if possible.

All bugfixes require a test case which demonstrates the defect. The
test should *fail* before the change, and *pass* after the change.

### Building documentation

Create a file with the name ```.release.json``` and put your GitHub token
into it so that it be read with JSON.parse:
Copy link
Member

Choose a reason for hiding this comment

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

This needs instructions for generating a github token. It also needs to be made clear that this is part of the release procedure, and that for local builds you can just run node docs/server.js.

P.S. this process extremely likely to change in the near future.


```
"abc123..."
```

Run ```node release.js``` which will build it from the "docs" directory and then commit it to gh-pages automatically.

### Releasing

Open an issue with a proposed changelog and semver-compatible version number.

Once this has been approved by the Collaborators, run ```npm prepublish```,
update ```History.md``` with the new changelog and bump the version number in
```package.json``` and ```component.json```.

Commit these changes and run ```npm publish```.
Copy link
Member

Choose a reason for hiding this comment

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

I've also been adding a git tag for each release. I use the component-release CLI tool for this process, but I'm not suggesting we force everyone to use that.


### I just made a mistake

With git, there's a way to override remote trees by force pushing
(`git push -f`). This should generally be seen as forbidden (since
you're rewriting history on a repository other people are working
against) but is allowed for simpler slip-ups such as typos in commit
messages. However, you are only allowed to force push to any jade
branch within 10 minutes from your original push. If someone else
pushes to the branch your commit lives in or the 10 minute period
passes, consider the commit final.
Copy link
Member

Choose a reason for hiding this comment

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

I think force-push is ok in feature branches?

125 changes: 125 additions & 0 deletions CONTRIBUTING.md
@@ -0,0 +1,125 @@
# Contributing to jade

Copy link
Member

Choose a reason for hiding this comment

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

We should include a section on asking for help in this guide. I'd like to make it clear that it's OK to open an issue for any communication related to jade and that issues are not just for bugs. It would also be good to have something to encourage people to a) search the docs first and b) post the entire error message, log and source code for reproducing any bugs.

### Step 1: Fork

Fork the project [on GitHub](https://github.com/jadejs/jade) and check out your
copy locally.

```text
$ git clone git@github.com:username/jade.git
$ cd jade
$ git remote add upstream git://github.com/jadejs/jade.git
```

#### Which branch?

For developing new features and bug fixes, the `master` branch should be pulled
and built upon.
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually going to be non-obvious for us? node has a much more complex branching strategy than I think we do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? Right now, we have 13 branches, including a v2. My hope is that one more line here might save us an issue asking this very question in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough


### Step 2: Branch

Create a feature branch and start hacking:

```text
$ git checkout -b my-feature-branch -t origin/master
```

### Step 3: Commit

Make sure git knows your name and email address:

```text
$ git config --global user.name "J. Random User"
$ git config --global user.email "j.random.user@example.com"
```

### Step 4: Rebase

Use `git rebase` (not `git merge`) to sync your work from time to time.

```text
$ git fetch upstream
$ git rebase upstream/master
```

### Step 5: Test

Bug fixes and features **should come with tests**. Add your tests in the
test/ directory. Look at other tests to see how they should be
structured (license boilerplate, common includes, etc.).

```text
$ npm test
```

Make sure that all tests pass. Please, do not submit patches that fail.
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure about this. I'd be happy to see pull requests as RFCs with failing tests.



### Step 6: Push

```text
$ git push origin my-feature-branch
```

Go to https://github.com/yourusername/jades and select your feature branch.
Click the 'Pull Request' button and fill out the form.

Pull requests are usually reviewed within a few days. If there are comments
to address, apply your changes in a separate commit and push that to your
feature branch. Post a comment in the pull request afterwards; GitHub does
not send out notifications when you add commits.


## Developer's Certificate of Origin 1.0

By making a contribution to this project, I certify that:

* (a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license indicated
in the file; or
* (b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source license
and I have the right under that license to submit that work with
modifications, whether created in whole or in part by me, under the
same open source license (unless I am permitted to submit under a
different license), as indicated in the file; or
* (c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified it.


## Code of Conduct
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 suggest to replace it with this.

I remember discussions about adopting Rust CoC in node.js last year, and community found too many flaws in it (for example "we don't tolerate behavior that excludes people in socially marginalized groups" implies "hey you can exclude other people all you want, just avoid excluding those ones"). Conference code of conduct is much more neutral in that respect.

Copy link
Member

Choose a reason for hiding this comment

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

I have no personal opinion on this and would defer to those in marginalised groups and those who have spent more time thinking about the issues. I do think it's important to consider what actions we are willing to take and in what circumstances, especially with regards to harassment that happens offline/in other unrelated spheres.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tough. I think the important thing to remember here is to use decency and common sense over the written word.
Personally, I prefer being explicity about including socially marginalized groups as an example (as referenced by "In particular, ..."). This makes it clear where our values lie, whereas a neutral statement could be seen as unclear. But ultimately we will only really prove ourselves by how we handle future incidents.


This Code of Conduct is adapted from [Rust's wonderful
CoC](http://www.rust-lang.org/conduct.html).

* We are committed to providing a friendly, safe and welcoming
environment for all, regardless of gender, sexual orientation,
disability, ethnicity, religion, or similar personal characteristic.
* Please avoid using overtly sexual nicknames or other nicknames that
might detract from a friendly, safe and welcoming environment for
all.
* Please be kind and courteous. There's no need to be mean or rude.
* Respect that people have differences of opinion and that every
design or implementation choice carries a trade-off and numerous
costs. There is seldom a right answer.
* Please keep unstructured critique to a minimum. If you have solid
ideas you want to experiment with, make a fork and see how it works.
* We will exclude you from interaction if you insult, demean or harass
anyone. That is not welcome behaviour. We interpret the term
"harassment" as including the definition in the [Citizen Code of
Conduct](http://citizencodeofconduct.org/); if you have any lack of
clarity about what might be included in that concept, please read
their definition. In particular, we don't tolerate behavior that
excludes people in socially marginalized groups.
* Private harassment is also unacceptable. No matter who you are, if
you feel you have been or are being harassed or made uncomfortable
by a community member, please contact one of the channel ops or any
of the TC members immediately with a capture (log, photo, email) of
the harassment if possible. Whether you're a regular contributor or
a newcomer, we care about making this community a safe place for you
and we've got your back.
Copy link
Member

Choose a reason for hiding this comment

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

This needs at least some re-wording. We don't have a technical committee or channel ops.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we ask them to open an issue instead? It would be public, but we don't have an IRC channel or similar, so I don't see an alternative.

* Likewise any spamming, trolling, flaming, baiting or other
attention-stealing behaviour is not welcome.
* Avoid the use of personal pronouns in code comments or
documentation. There is no need to address persons when explaining
code (e.g. "When the developer")