-
Notifications
You must be signed in to change notification settings - Fork 11
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
Style #8
Style #8
Changes from 17 commits
1b5a467
cd5ca14
e7a5305
f6c5639
488f276
953c856
b6712f3
aceab03
6563128
08d1e87
ee60000
84d7753
2fa3b13
808f4ff
765b53e
8ebce32
6f0e02c
99ed3d2
dbf767c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
# Style | ||
|
||
This document explains the standards we have around code style in various different languages. | ||
Style is important to standardize for a few reasons: | ||
|
||
* Makes code more readable, and thus easier to understand. | ||
* Avoids holy wars over small details, but still encourages an approach to coding that sweats the details. | ||
|
||
For each of the languages below we link to a style guide reference and outline tools to use in order to help follow the style guide. | ||
Making sure you follow these guidelines is at the same level of importance of making sure tests pass, so the tools outlined below should be part of the build and test cycle for every project. | ||
Doing it as part of the build and test cycle (as opposed to elsewhere, e.g. git hooks) ensures that no code is merged into master that fails these checks and allows for maximum flexibility during development. | ||
As a code reviewer, you should hold people accountable to following the style guide for the appropriate language. | ||
|
||
## Go | ||
|
||
The style guides used for Go are [Effective Go](http://golang.org/doc/effective_go.html) and the community [Go style guide](https://code.google.com/p/go-wiki/wiki/CodeReviewComments). There are two tools that can be used to detect common mistakes. | ||
|
||
* `go fmt` should be run in any directory that contains go files. It will automatically format the code. | ||
* `golint file.go` should be run for every go file. It will lint the code and return any issues it finds. | ||
|
||
### Recommended setup | ||
|
||
* Makefiles: A Go package should have a Makefile that runs "golint" on all files, e.g. | ||
|
||
```Makefile | ||
$(PKG): | ||
ifeq ($(LINT),1) | ||
golint $(GOPATH)/src/$@*/**.go | ||
endif | ||
go test | ||
... | ||
``` | ||
See [clever-go](https://github.com/Clever/clever-go/blob/master/Makefile) for an example. | ||
* emacs: Go has an official [emacs mode](http://golang.org/misc/emacs/go-mode.el) that ships with a `gofmt` command. To get it to run on save, you can add this to your `.emacs`: | ||
|
||
``` | ||
(add-hook 'before-save-hook 'gofmt-before-save) | ||
``` | ||
|
||
* sublime: Add [GoSublime](https://github.com/DisposaBoy/GoSublime) for code highlighting and `go fmt` on save. | ||
|
||
* vim (TODO) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
## CoffeeScript | ||
|
||
The style guide used for CoffeeScript can be found [here](https://github.com/Clever/coffeescript-style-guide). [coffeelint](https://github.com/clutchski/coffeelint) should be added as a test. The coffeelint config can be found [here](https://github.com/Clever/coffeescript-style-guide/blob/master/coffeelint-config.json). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about coffee-jshint? as it stands, coffee-jshint catches some issues that coffeelint doesn't cover yet (like undefined variables) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jefff can you add coffee-jshint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
## Python | ||
|
||
The style guide we use is [PEP8](http://legacy.python.org/dev/peps/pep-0008/) with exceptions for allowing tab widths of two spaces and line lengths of up to 100. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we allow tab widths of two spaces, or do we mandate it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm of the opinion we should allow it, but encourage PEP8's four spaces moving forward. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a strong PEP8 enthusiast, but would rather have one format than changing it. I think it's totally fine to go with 2 spaces (mandating). |
||
|
||
### Recommended setup | ||
|
||
PEP8 has an accompanying command-line tool, `pep8` (`pip install pep8`) that accepts a config file: | ||
|
||
``` | ||
[pep8] | ||
ignore = E111 | ||
max-line-length = 100 | ||
``` | ||
|
||
There is also the tool `autopep8` (`pip install autopep8`) that will fix all the problems found by `pep8`. | ||
|
||
* emacs: You can run `autopep8` on save by installing [`py-autopep8`](https://github.com/paetzke/py-autopep8.el) and adding the following to your `.emacs`: | ||
|
||
```Makefile | ||
(add-hook 'before-save-hook 'py-autopep8-before-save) | ||
(setq py-autopep8-options '("--max-line-length=100" "--indent-size=2")) | ||
``` | ||
|
||
* vim: https://github.com/tell-k/vim-autopep8 | ||
|
||
* sublime: https://github.com/wistful/SublimeAutoPEP8 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we suggest pylint, pyflakes or another tool? I like the look of https://github.com/klen/pylama which wraps 'em all @schimmy @mohit do you have a preferred python linter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider for sublime... https://github.com/SublimeLinter/SublimeLinter3 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nathanleiby I'm of the opinion we should keep the list of tools relatively small to begin with since (1) we have a lot of work to do just to get up to PEP8, and (2) to keep the setup simple in our dev and testing environments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I weakly agree with @rgarcia. Nonetheless my preferred tool is pyflakes (which does pep8 checks also). Never used pylama. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rgarcia Agreed, would like to keep things simple. However, for me having suggested tools that permit following these rules is helpful. Another option (that puts the burden on the repo instead of on the developer's setup) could be for repos to have a test which runs pep8 against the repo's files. Therefore breaking pep8 -> a failing test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nathanleiby @mohit does pyflakes do auto-correction? What I like about pep8 is that autopep8 can be configured to run on save and fix common errors (similar to gofmt). @nathanleiby re testing: the general approach this doc recommends is having checks run alongside tests (see the introduction), however the approach does not go as far as making the tests fail. I think step one is getting all of our repos to a place where these checks are easy to run and run successfully. My ideal world is one where there's a separate commit status reported for running these checks, which essentially elevates them to the status of a test failure. |
||
## Bash | ||
|
||
The [Google Shell Style Guide](https://google-styleguide.googlecode.com/svn/trunk/shell.xml) is what we follow for shell scripts. | ||
There are no automated tools that we use for following this, however there is [ShellCheck](http://www.shellcheck.net/about.html) which covers some of what's in the style guide. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh this is gonna be helpful! didn't know anybody had even thought about style for shell scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to guide on code reviewing? err, guess we don't have one. do we need one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need one: #9