Loomio

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.

F

Faldrian Sun 22 Feb 2015 11:31AM

120 width is great :)

I don't want assignments in if statements, you rarely need them directly and you could assign the result of the assignment to a variable and then check the variable ... this way the reader can not confuse this easily with the tests in if statements.

For the rest: I have no opinion and trust you do some sane decisions. :)

SVB

Steffen van Bergerem Sun 22 Feb 2015 12:44PM

(changed the description of the discussion, added information about the javascript styleguide)

JH

Jonne Haß Sun 22 Feb 2015 5:27PM

Oh, mh, couldn't everybody edit the start of the discussion at the top?

SVB

Steffen van Bergerem Sun 22 Feb 2015 5:54PM

@jhass done.

DU

Sun 22 Feb 2015 9:43PM

About JS camel case, I like the PEP8 i.e : CamelCase for classes or objects or others, snake_case for functions with the minimum length for function name. I don't like the idea of auto-documented code. The actual code (espacially JS) should be systematically documented using /** */ coments.

JH

Jonne Haß Sun 22 Feb 2015 10:34PM

auto-documented code

You mean self-documenting code? If so, any reason? What's wrong with code that's self explanatory? And what's wrong with spending effort on making code clearer than spending that effort on explaining your incomprehensible code instead and making sure the comments stay up-to-date? Do you never experience comment rot?

DU

Sun 22 Feb 2015 10:38PM

You mean self-documenting code? If so, any reason? What’s wrong with code that’s self explanatory?

A code is never self-explanatory. It can be comprehensible by someone with sufficient knowledges in a particular language when it's well-written, but that is not self-explanatory, that is just good programming.

But when someone not really comfortable with a language reads it, a code is not self explanatory. This is why we always continue to write documentations...

And what’s wrong with spending effort on making code clearer than spending that effort on explaining your incomprehensible code instead and making sure the comments stay up-to-date?

Can't we do bot ?

Do you never experience comment rot?

Not more than I experience code rot.

JH

Jonne Haß Sun 22 Feb 2015 10:57PM

But our code is not to teach you the language, we're writing a software here, not a book that explains the language or a sample application for someone learning the language.

Not more than I experience code rot.

Lucky you then! I can't say that's my experience, in fact I've seen a lot in diaspora itself.

F

Faldrian Sun 22 Feb 2015 11:02PM

Make comments when necessary, documenting things that may be solved differently and explain why it was done this way.

I think these styleguides set the "must have" part - everyone should still feel free to add a comment to newly written code if it is necessary but not demanding comments when they are not needed.
More important: If you change something, validate that comments are still accurate, so no one reads comments and thinks "okay, I understood what this does" - but the code was changed in between and does something completely different now.

DU

Sun 22 Feb 2015 11:21PM

But our code is not to teach you the language, we’re writing a software here, not a book that explains the language or a sample application for someone learning the language.

Common, you know as well as me that it's not about the language. See, even Java devs who I don't think they are bad document their code a lot.

I'm with @faldrian here :

Make comments when necessary, documenting things that may be solved differently and explain why it was done this way.

We all know that diaspora* is not documentaed enough and new devs will never come if this is so difficult to understand the code structure. There are sometimes legacy things that appear to be crazy, espacially in the JS code. Don't forget that we don't code for us, but for those that will come after us.

Lucky you then! I can’t say that’s my experience, in fact I’ve seen a lot in diaspora itself.

There should be a right balance. At leat every JS function should be documented with /** */ comments and two or three lines. Just see JSXC source. It is highly commented and thanks to this easy to modify and maintain.

I think, from now, at least every new JS contribution has to be commented.

Load More