In both of your examples I see 40-60% of the comments as superfluous. I mean things like /** Some constants */? Or /** My bar id */, bid: null,, here the time would've been better spend by using a self-explanatory name, like bar_id. Most of the comments of your Java example are for the API docs, not to explain the actual code or even the classes structure.

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

Java styleguides often have such a requirement, and I know enough developers who don't like it. Therefore I highly disagree with such a requirement. Giving an overview at the top of your class / file about its purpose can be good if it's non-obvious, but it should be applied as necessary, not as necessity. The later only leads to superfluous noise in the code, like in the examples I pointed out (which I didn't even have to scroll down a page for).

In both of your examples I see 40-60% of the comments as superfluous.

That's what I say : you see. But you don't code for yourself, you code for every other developer of diaspora*.

A code is never self-explanatory. It might be for you, but I'm sorry to say that, given a complex function, a name with two or three words is never enough. Especially in JS.

and I know enough developers who don’t like it

Oh ! This is an actual study from the institute of the wet finger !?

At work, I'm responsible for the documentation of all the projects. I see every day devs who were convinced their code was self-documented, no need for comments or actual document. And see, 5 years later, they are unable to say what their code do, what they did it for. For some project, we are even unable to say what they are used for and if they are stil up-to-date ! Documentation and comments are not luxury.

Giving an overview at the top of your class / file about its purpose can be good if it’s non-obvious

Who determines it is non-obvious ? We have to be clear : wether we do, wether we don't. Where you see superfluous comments, I see a sane way of coding where comment helps to see the logic and the structure of the code.

When you want to know how something specific works, you will always read the code.

But what we really would need (and I think this is more important than inline-comments or class-comments) is a top-down overview of the whole project, maybe with 2-3 different zoom-levels, explaining how everything works together (graphically with annotations). That would not be part of this styleguide-thing, but a new piece of works.

@augier I don't think we have to introduce mandatory comments here. If you think they are useful and want to add some - you can, but you don't have to.

I guess we have to put that point to vote then, let me answer to one more of your points though:

given a complex function, a name with two or three words is never enough.

The problem in that case is not that you have no comment on it, the problem is that you didn't break up your function. I only have a Ruby example at hand, but let's take it. This method calls a bunch of other methods that aren't used anywhere else, they're extracted not for reusability but for readability and to reduce the size of the method.

I can recommend this book, it has an excellent chapter on comments too.

But what we really would need (and I think this is more important than inline-comments or class-comments) is a top-down overview of the whole project, maybe with 2-3 different zoom-levels, explaining how everything works together (graphically with annotations). That would not be part of this styleguide-thing, but a new piece of works.

Agree.

@faldrian : I don't have strong opinion about Ruby, but concerning JS, and given how this language is unnecessarily verbose, I really think we should have document comments for each function more than 4 lines. And there is a lot of them.

The problem in that case is not that you have no comment on it, the problem is that you didn’t break up your function.

This is also a problem, for sure. The there are many complex function of that kind in the JS part. And we all know that they will never be refactored.

Your Ruby example is not fully relevant as Ruby is a very concise and easy to read, which is not the case of JS.
Please also notice that it has many doc comments and is not as difficult to read as you say.

Also, I think we should do an inventory of the SCSS files during the port to BS3.

as Ruby is a very concise and easy to read, which is not the case of JS.

I think your milage may vary. I find ruby very disturbing to read and feel like in wonderland when I can read JS. So this does not depend on the language.

Also the comment in my example is for the API docs, the extracted private API has no comments at all.

About quotmark, I've adopted the styleguide of Ruby mine for both JS and Ruby, i.e : ' ' by default and " " only when necessary.

In Ruby, I found myself repeatedly changing 's to "s since I wanted to use string interpolation or have an interpreted \n and so on, therefore I adopted to default to "s and only use ' if I need to their semantics. Any such reason for preferring ' in JS?

Of course! :D https://twitter.com/mathias/status/247708186696634368

(No, there is not even a performance difference ... http://jsperf.com/single-quote-vs-double-quote/3 )

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.

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

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.

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

So, shall we put mandatory comments to a vote?

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

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.

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.

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 ?

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

It's just a proposition, I don't see any other easy way to measure it. It's not useful to put mandatory comments for very obvious case, but we need a clear rule to determine what is ovious and what is not.

I don't like rules and hard limits where none are necessary. It is already possible to look through new pull requests and add comments like "hm... big block of code, care to comment some of that?" - if the developer not already has done so.
We all want to write good and understandable code - if there are people who have hints how to make code better understandable, they can say so in the PRs.

I think the solution to documentation issues will more likely be solved by communication and external documentation than by enforcing arbitrary rules on the structure of new code.

Let's focus more on gettings things done here than setting up a ruleset to play by... (that won't be used if people don't see the purpose of it).

I'm totally with Faldrian. Since there obviously are no simple rules to determine when a mandatory comment should be placed, do you still hold to wanting them?

We have to find how to better document the code. This is the solution I found. If I'm the only one on this, of course it will never be accepted.

But the problem is still here : the code is still abstruse. So, no matter what, we need a solution on this.

When the styleguide is set, what if I set up a RubyMine inspection file for the official repo ?

But the problem is still here : the code is still abstruse. So, no matter what, we need a solution on this.

The way forward is to make it less abstruse, not to bury the clear parts in comments.

When the styleguide is set, what if I set up a RubyMine inspection file for the official repo ?

I don't use RubyMine, so maybe. We will most likely add the above mentioned linters with configs that match the styleguide as good as possible and enable Hound.

I don’t use RubyMine, so maybe. We will most likely add the above mentioned linters with configs that match the styleguide as good as possible and enable Hound.

BTW, RM supports JSHinter and JSLinter.

JSLint is a bunch of cr*p. It's too harsh for any sane person. JSHint is the way to go, I guess. I think comments should be optional, but each file should at least have a comment, explaining what it's trying to do in the system, why it's needed there. But this can be enforced when accepting PR's, there's no need for a linter to check it.

Regarding SCSS - It's a mess right now. I've been thinking a lot about it, and it's a difficult problem.

I propose to use the SMACSS approach and use these rules to separate the selectors:
https://smacss.com/book/categorizing

We should have 5 folders and a file:

• base

  • _base.scss (only for including this folder files)
  • random .scss files

• layout

  • _layout.scss
  • the same rules as before

• module

• state

• themes

  • default (a folder)
  • _main.scss (to include the other files) (this should be nested under default folder, it's something weird with loomio markdown)
  • whatever files the theme creator wanted, it may again follow the SMACSS structure

• application.scss

  @import 'base/base', 'layout/layout', 'module/module', 'state/state';
  @import 'themes/cerulean/_main'; (changed the theme to cerulean)

@dumitruursu We already use JSHint.

Since there's no discussion around it, I will start the vote for the Ruby styleguide tomorrow, unless discussion emerges until then.

@dumitruursu I recently found Kernel#abort, which is the official way to abort the program with an error state.

hmm, that is actually not bad.

In the proposed settings for jshint I changed
camelcase : true: we already use camelcase for most backbone.js functions
quotmark: 'single': no need to escape double quotes in HTML
maxlen: 120

I will create a proposal for the javascript styleguide as soon as the ruby proposal closes.

So there's no possibility to differentiate camelCase for class names and function and snake_case for local variables ? Well. I'll deal with it.

In addition, I'd propose to use JS Code Style to enforce some rules like no trailing spaces.

In addition, I’d propose to use JS Code Style to enforce some rules like no trailing spaces.

It is absolutely unnecessary to use two tools doing basically the same. JSHint does everything we need. Trailing whitespace should be removed anyway, but that's nothing JS specific. ;)

In addition, I’d propose to use JS Code Style to enforce some rules like no trailing spaces.

I agree. The JSHint docs say that some options will be removed in the next major release and that you should use JSCS for those instead. Unfortunately Hound doesn't support JSCS yet and as far as I know there is no Ruby gem for JSCS.

@dennisschubert : as @steffenvanbergerem said, the options related to code style are going to be deprecated in the next release of JSHint. They say that JSHint is only about code correctness and not code style. JS has a lot of ways to do some things and I saw very different ways of developing while working on the notifications system. We need to agree on things like using var that = this; or rather var self = this;, spaces usage (before and/or after brackets and curly braces?) and so on. Every big project has a code style. It improve the understandability of the code as it provides conventions.

@dennisschubert @jasonrobinson If you want we can have another proposal after the current one to decide whether we prefer single or double quotes. In that case I would keep the current value (false, accept single and double quotes) until we have an outcome for the second proposal.

It would be a bad idea to enforce single quotes if the majority prefers double quotes.

For (S)CSS I suggest adopting the coding styles from Bootstrap, which are very reasonable, and will provide enough structure to make the code consistent.

Also, it will be very useful to have a similar notation on the bootstrap styles and our own, with no modular__classNaming--nonsense.

http://codeguide.co/#css

I think it will be pretty easy to adopt too.

This is only for the "measurable" styleguide, there are also some best practices we should adopt, but those will be our own, and we can add best practices gradually on the run as we find use cases. I have a few of my own to suggest.

@steffenvanbergerem if you guys want me to create a separate proposal, just let me know. I see this one is closed, but I guess the discussion is still alive.

A discussion can totally have multiple proposals, we already used this one to decide Ruby and Javascript. Feel free to edit the top post accordingly too to reflect current discussion status.

I'd like to enforce the styleguide with HoundCI, at least as much of it as possible. It runs SCSS-Lint, do you happen to run across an existing config for the guide you propose?

Nope, but I'll give it a try.

There are some further insights on the approach that gave birth to that styleguide here, the guys at Github use SCSS and the linter too:

http://markdotto.com/2014/07/23/githubs-css/#linting

I still have to check some config values to see if they are fully compatible with the guide I posted, but for anyone interested in taking a look, here is the ANSI-colored output of SCSS Lint with Hound's config, which I think is pretty close to the guide:

http://www.vispress.com/scss-lint-hound.txt

And here are the differences between the default scss-lint config and Hound's:

https://www.diffchecker.com/n2wj07ys

I think maybe we should disable some rules, or apply some automatic styling, even using sed...

As I pointed it here one of the things that gave me hard time whil porting to BS3 was the fact that classes where used both to define CSS rules and to select in JS.

As most of the CSS rules defined could be removed because they were alreay defined by BS, I nerver knew if the class ws safe to remove or if it was still used by any JS somewhere.

So, my idea would be, from now, to avoid using the same classes to define CSS rules and to select in JS. The diference could be pointed by the use of a prefix on the classes used by JS like js-. This will avoid to break logic or unit tests in the future when removing deprecated classes.

For SCSS I think we could start with the default config of scss-lint. Any comments before I create a new proposal?

Does someone knows a JSLinter to enforce some programmatic rules like do not use parseInt without base or do not use for(... in ...) loop?

I've discovered here that parseInt was used without giving a base, for instance.

Hey, I'd like to discuss about the two rules:
- to not use ids selector
- to not have a depth of 4 or more

I know ids selector are bad for performance, but I'm not sure that we need to be that strict about it.
And I would like to know why it's bad to have a big depth? Scss is compiled anyway, I'm not sure why we want to avoid that, it's even useful to avoid side effect.

And I would like to know why it’s bad to have a big depth? Scss is compiled anyway, I’m not sure why we want to avoid that, it’s even useful to avoid side effect.

As Steffen indicated me tonight, the given reason is that deep depth lead to a CSS tightly-coupled to your HTML structure.

Though, I tend to agree with you: I think the depth could be extended to 5.

Same about ID selectors. IMHO, there are legit use case.

You can find reasons for these rules in the scss-lint documentation.

IdSelector
SelectorDepth