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

CONTRIBUTING.md and CODE_OF_CONDUCT.md #116

Merged
merged 4 commits into from
Feb 21, 2017
Merged

Conversation

liclac
Copy link
Contributor

@liclac liclac commented Feb 8, 2017

We need contribution guidelines and a code of conduct before we start picking up any more contributors. I'll look into automatically enforcing these guidelines later - there are several bots that can do it.

(Ping @pnn in particular)

@liclac liclac changed the title [docs] CONTRIBUTING.md and CODE_OF_CONDUCT.md CONTRIBUTING.md and CODE_OF_CONDUCT.md Feb 8, 2017
@ragnarlonn
Copy link

Nice! I just have two comments:

"until it's polished and perfect" - I don't like that wording. It gives a sense that we strive for perfection, which is never a good idea. I suggest "until it becomes good enough for inclusion into the code base", or similar.

On the commit format - quite a lot of commit prefixes people have to learn, and understand when to use. I suggest replacing "all commits MUST have..." with "all commits SHOULD have...". We should not force one-time or new contributors to learn a lot of complex rules before they can even start contributing. That may scare people off.

I like the CODE_OF_CONDUCT also.

CONTRIBUTING.md Outdated

(ノ◕ヮ◕)ノ*:・゚✧

Before you begin, make sure to familiarize yourself with the [Code of Conduct](CODE_OF_CONDUCT.md). If you've previously contributed to other open source project, you may recognize it as the classic [contributor covenant](http://contributor-covenant.org/).
Copy link

Choose a reason for hiding this comment

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

other open source projects 😉

Copy link

Choose a reason for hiding this comment

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

also i'd capitalize Contributor Covenant

CONTRIBUTING.md Outdated

Don't be afraid to file issues! Nobody can fix a bug we don't know exists, or add a feature we didn't think of.

The worst that can happen is that someone closes it right away and points you in the right direction.
Copy link

Choose a reason for hiding this comment

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

closes it right away sounds a bit harsh, let's keep it at just closes it

CONTRIBUTING.md Outdated

3. Create a pull request - make sure you make it from your feature branch to develop!

4. We well discuss implementation details until it's polished and perfect, then a maintainer will merge it.
Copy link

Choose a reason for hiding this comment

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

we will

CONTRIBUTING.md Outdated
4. We well discuss implementation details until it's polished and perfect, then a maintainer will merge it.

We use [git flow](http://nvie.com/posts/a-successful-git-branching-model/), so you may recognize our branching structure from, well, every other project that does this. If not, have a look at that post and you'll feel right at home in no time.

Copy link

Choose a reason for hiding this comment

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

ideally, there should be a really basic explanation of git flow for folks that don't want to read a whole blog posts here. besides, only the concept of the develop branch and feature branches are relevant for this section

CONTRIBUTING.md Outdated

We use [git flow](http://nvie.com/posts/a-successful-git-branching-model/), so you may recognize our branching structure from, well, every other project that does this. If not, have a look at that post and you'll feel right at home in no time.

Style guide
Copy link

Choose a reason for hiding this comment

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

this could be an anchor link

CONTRIBUTING.md Outdated
```
[feat] Added this really rad feature

Closes #420
Copy link

Choose a reason for hiding this comment

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

nice

CONTRIBUTING.md Outdated

Any human-readable text you add must be non-gendered (if applicable), and should be fairly concise without devolving into grammatical horrors, dropped words and shorthands. This isn't Twitter, but don't write a novel where a single sentence would suffice.

If you're writing a longer block of text to a terminal, wrap it at 80 characters.
Copy link

Choose a reason for hiding this comment

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

this could be clarified, i feel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this?

@ghost
Copy link

ghost commented Feb 8, 2017

i just sort of wrote some notes down. besides those, i agree with what @ragnarlonn said

CONTRIBUTING.md Outdated

Remember, there's more to software development than code; if it's not properly planned, stuff gets messy real fast.

2. Create a fork and open a feature branch based off develop - `feature/my-cool-feature` is the classic way to name these, but it really doesn't matter, as long as you don't hack directly on develop.
Copy link

Choose a reason for hiding this comment

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

Wait, what? "develop" ? What's that branch and why is that used instead of master? That's highly unorthodox and if you'll keep that system I think you need to explain that here first and not take it for granted. Most git projects out there just use master as... eh, the master branch.

@bagder
Copy link

bagder commented Feb 9, 2017

Is there some considerations regarding copyright that contributors should be aware of or adhere to?

@martinfijal
Copy link
Contributor

To make it simpler for people wanting to contribute, I'd make the following changes:

  • Drop the use of commit prefixes. We've used them internally for a while and I've yet to see any actual benefit to them.
  • Drop the use of git flow and just accept pull requests straight to master.

@robingustafsson
Copy link
Member

Nice @uppfinnarn!

I agree with @bagder's and @martinfijal's line of thinking that git flow is perhaps overkill for k6 and adds an obstacle for people wanting to contribute. I'd also be in favor of just accepting pull requests against master and then making sure releases are managed properly (tagging, changelog etc.).

On the subject of releases, we should perhaps state somewhere what the release policy is? Fixed cadence? Whenever a maintainer feels like it? That's something I look for when contributing code. When will I be able to use an official release rather than my branch/packaging of the lib/app.

I think commit message prefixes are a good thing as they help when scanning a commit list for particular types of commits or areas of change. Also handy when putting together changelog/release notes.

What about an AUTHORS/CONTRIBUTORS file or something to recognize contributors? Or is that something best left for release notes?

@ghost
Copy link

ghost commented Feb 9, 2017

@robingustafsson regarding contributor lists, there's multiple ways to go about it. you could obviously keep a CONTRIBUTORS.md file in the project root, but personally i think it'd be better to manually keep a list of people that contribute frequently in the README.md, akin to how the node.js project does it.

@bagder
Copy link

bagder commented Feb 9, 2017

For my projects, I've found that it makes a lot of sense to write the commit messages' first line pretty much as a line that perhaps could make it into the RELEASE-NOTES (since then it goes fast to generate that list). In the curl project, we then put that into a web page for every release.

Having the first line < ~60 columns is also good for when you do git blame etc on the code.

A recent example:

http_proxy: Fix tiny memory leak upon edge case connecting to proxy
    
Fixes #1255

The commit message template for the curl project says this:

---- start ----
[area]: [short line describing the main effect]
       -- empty line --
[full description, no wider than 72 columns that describe as much as
possible as to why this change is made, and possibly what things
it fixes and everything else that is related]
       -- empty line --
[Bug: URL to source of the report or more related discussion]
[Reported-by: John Doe - credit the reporter]
[whatever-else-by: credit all helpers, finders, doers]
---- stop ----

... then we have a script that automatically gathers all Authors, Committers and people mentioned in the above mentioned *-by:- lines and update the THANKS file accordingly based on that.

Me personally don't believe in separating frequent contributors from others in the THANKS/CREDITS file. Everyone who helps out deserves a mention. A single change can be worth more than 10 separate ones.

This is just input on what has worked for us. Not implying these must be followed by you.

@ghost
Copy link

ghost commented Feb 9, 2017

yeah - on second thought, maybe it is better to list all contributors. the single-out-proficient-contributors approach would be better for projects with larger (think >500) contributors, i guess?

also, if we're sharing commit message strategies, on the jekyll project we generate a history file based on the pull request titles. the history document is altered by a bot on every branch merge. here's an example of this.

that all depends on how releases are being done though? a dedicated bot or script might be overkill at first

@ragnarlonn
Copy link

Just a general reflection: I think that last comment by @pnn touched on something that might be important to remember - if you're used to working in a large project (in terms of contributor numbers or lines of code) the processes used there may not be ideal for k6 at this stage. It is usually wise to keep things very simple at the beginning of any project, only adding complexity as needed.

@liclac
Copy link
Contributor Author

liclac commented Feb 10, 2017

There are some good points here, the big thing is that master should be kept usable, but looking at how we handle it right now, we just tag lots and lots of small releases to merge develop into master all the time.

Would everyone be for dropping the develop/master split, while still keeping the pull request model for new stuff?

@liclac
Copy link
Contributor Author

liclac commented Feb 16, 2017

How's this? When we merge, I'll get rid of develop and start taking PRs against master.

@ragnarlonn
Copy link

ragnarlonn commented Feb 20, 2017

One very minor detail: we ask people to limit length of output lines (to terminal) to 80 characters, but maybe we should go for something slightly shorter. @bagder mentioned ~60 chars for commit messages and it appears that is about the recommended line length for best reading ergonomics - (https://en.wikipedia.org/wiki/Column_%28typography%29) Another benefit is that choosing a value a bit below 80 means people will be able to e.g. indent (for presentation purposes) k6 output with a few spaces without causing linebreaks on an 80-char display device.

@bagder
Copy link

bagder commented Feb 20, 2017

We usually wrap commit messages at 72 actually, since "git log" in the terminal indents the commit messages a bit.

@liclac
Copy link
Contributor Author

liclac commented Feb 20, 2017

Hm, I usually prefer everything to be close to 80ch wide (accounting for indentation). Reading ergonomics is something very complicated (…that I've sunk an unhealthy amount of time into…), and varies by far more factors than just the width.

@ragnarlonn
Copy link

78? 72? I just think that if we aim to be 80x25-friendly we don't want to use 80-character lines when we output things. If we don't care about 80x25 (and frankly, is that important today?) we can choose whatever we want of course (though much longer than 100 chars is probably not good for reading ergonomics).

@liclac
Copy link
Contributor Author

liclac commented Feb 20, 2017

Mostly, all the terminals I know of (gnome-terminal, xfce4-terminal, iTerm2, Terminal.app, cmd.exe, I believe PowerShell too?) default to 80 characters wide.

But maybe leaving some margin wouldn't be too bad?

@liclac liclac merged commit 8b298c5 into master Feb 21, 2017
@liclac liclac deleted the feature/CONTRIBUTING.md branch May 17, 2017 12:18
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.

5 participants