Loomio
Sun 22 Feb 2015 11:13AM

Styleguides

JH Jonne Haß Public Seen by 69

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 for raise 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 disallowing if 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, not Foo = Class.new(RuntimeError). The later requires more knowledge about Rubys meta model and what the argument to Class.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 use alert and confirm 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 to true.
  • 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.

http://codeguide.co/#css

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.

DU

Deleted account Mon 23 Feb 2015 6:41PM

Any such reason for preferring ' in JS?

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 :

var object = {
    var1: 'var',
    var2: 'var also'
}

Function definition :

var object = function(){
    var1 = 'var';
    var2 = 'var also;
}

Prettier and easier to read.

DU

Deleted account Tue 24 Feb 2015 3:55PM

Would be nice to set option multistr to true, Milou bothers me with that >.<

SVB

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.

DU

Deleted account Tue 24 Feb 2015 8:49PM

I didn't know that. Can't wait for ES6...

JH

Jonne Haß Thu 26 Feb 2015 3:50PM

So, shall we put mandatory comments to a vote?

F

Faldrian Thu 26 Feb 2015 3:55PM

Since we exchanged arguments with pretty much no effect, this might be a good idea.

DU

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.
JH

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.

DU

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 ?

JH

Jonne Haß Thu 26 Feb 2015 9:07PM

That's way too vague, you can't measure complexity in LoC or calls.

Load More