Styleguides
So looks like we want to establish coding styleguides and actually enforce them. Let's try to find one that we all like.
Tools
We recently enabled Hound for Javascript. That should settle JSHint as linter for Javascript. Hound supports Ruby and SCSS linting too, through Rubocop and and scss-lint respectively. Those seems to be flexible tools, so let's settle on these too and enable Hound for Ruby and SCSS too.
Ruby
Rubocop defaults to the bbatsov styleguide plus some additional checks not specified there. Hound changes some of Rubocops defaults, unless you specify your own config, to match the thoughtbot styleguide. I can not fully agree to either, however bbatsov's provides a good base, especially since Rubocops defaults are modeled after it, which makes it less work to configure. Therefore I started a Rubocop config that we can use as a base for discussion. I tried to add some reasoning to most entries. However let me point out the main choices and deviations from bbatsov here:
- Line length: 80 is a bit too short to enforce, especially since we have bigger screens these days. I'm working with 120 for years now and so far nobody complained.
- Different spacing rules for
{ ... }
blocks and hash literals, in order to differentiate them better - Using
fail
requires you to know that it's just an alias forraise
with no benefit, in other places the styleguide already discourages using aliases. Having more than one semantic meaning for exceptions is wrong in the first place, it suggests that you're using them for control flow. - No safe assignment, that is, do not allow
if (foo = bar)
while disallowingif foo = bar
. We should either allow assignments in if statements or not, this is just inconsistent. - Do not enforce block argument names for
inject
. Enforcing variable names, especially single letter ones is just plain silly.
Additional rules and deviations that I couldn't make out a cop for:
- Define exceptions as
class Foo < RuntimeError; end
, notFoo = Class.new(RuntimeError)
. The later requires more knowledge about Rubys meta model and what the argument toClass.new
does for basically no benefit. - Disallow spaces inside string interpolation that is allow
"a #{b} c"
but not"a #{ b } c"
. This is to settle to one style and keep it consistent. - Disallow doing control flow with
&&
and||
outside conditionals. The only valid uses of&&
and||
are in a condition and to compute a predicate that's returned from a method.
Please read bbatsov's styleguide and then my Rubocop config if you want to know more.
Javascript
(last edit by Steffen)
The proposed deviations for our settings are: (follow the links for more information about the options)
- camelcase :
true
- latedef:
true
- maxlen:
120
- nonew:
false
. nonew would prohibit the use of constructor functions for side-effects. While testing that setting to me it looked like we need those side-effects for our backbone code. Does anyone have more information about that? (I am no javascript/backbone.js expert) - quotmark:
'single'
- eqeqeq:
true
- freeze:
true
- indent:
2
- devel:
true
. We usealert
andconfirm
in some places. - eqnull:
true
. - lastsemic:
true
- supernew:
true
. We use that in some places. I don't know if we should get rid of it but because we are currently using it this is set totrue
. - I also added some global variables we are using.
SCSS
(edited by Pablo Cúbico)
I'm suggesting using the styleguide from Bootstrap, which is a little leaner than things like BEM and OOCSS.
Some ideas from its maker:
http://markdotto.com/2014/07/23/githubs-css/
I'll try to match the configuration of SCSS Lint to it and post it here.
Porting the codebase
We won't do it. As reasoned excellently in Hound's faq, the way to adopt a styleguide is to gradually port all code you write and touch to it, not to do it in one go.
Deleted account Tue 24 Feb 2015 3:55PM
Would be nice to set option multistr
to true, Milou bothers me with that >.<
Steffen van Bergerem Tue 24 Feb 2015 7:18PM
The JSHint docs say
Multi-line strings can be dangerous in JavaScript because all hell breaks loose if you accidentally put a whitespace in between the escape character () and a new line.
Deleted account Tue 24 Feb 2015 8:49PM
I didn't know that. Can't wait for ES6...
Jonne Haß Thu 26 Feb 2015 3:50PM
So, shall we put mandatory comments to a vote?
Faldrian Thu 26 Feb 2015 3:55PM
Since we exchanged arguments with pretty much no effect, this might be a good idea.
Deleted account Thu 26 Feb 2015 4:40PM
I think so.
To me, proposition should at least include these elements :
- mandatory comments required for eavery PR
- at least for JS code
- at least for functions that are above 3 lines.
Jonne Haß Thu 26 Feb 2015 6:13PM
mandatory comments required for eavery PR
at least for functions that are above 3 lines.
That's contradictory, please clarify.
at least for JS code
All or nothing, doing it for part of the code base and for another part not is just worse.
Deleted account Thu 26 Feb 2015 6:35PM
I say at least, now if we vote fore mandatory comments and you think it's better to
That’s contradictory, please clarify.
Every new function has to be documented unless it is obvious (less than 3 instructions) eg. this is obvious and this too.
I consider as an instruction any call of method. So, if there's chained methods call, each call is an instruction. Eg :
var.call1().call2().call3().call4()
is 4 instructions. But I'd like to limit a too big number of chained call (not more than 4, maybe ?) if possible.
Also, about SCSS is nesting limit relevant ?
Jonne Haß Thu 26 Feb 2015 9:07PM
That's way too vague, you can't measure complexity in LoC or calls.
Deleted account · Mon 23 Feb 2015 6:41PM
None, it is just personnal preference. And I prefer using
" "
for class or id when I define inline HTML, so this prevent quotes collision and usage of\"
.Edit : yeah, just like @faldrian :p
Also, about JS, I like JSON rather that
function()
for object definitions :JSON :
Function definition :
Prettier and easier to read.