JSCS config review

[Reinmar]

I reviewed today JSCS options and it turned out that due to changes in JSCS the config I created for CKE4 became outdated. Moreover, in CKE5 we will be able to follow more strict rules which we were not able to apply to an entire code base in CKE4.

Proposed config

This is what we'll have now in CKE4 plus some options which we cannot enable in CKE4 but I can be enabled in the boilerplate.

Note: Comments must be removed before pasting this in https://github.com/ckeditor/ckeditor-boilerplate/blob/master/dev/tasks/jscs-config.json. Although, I find those comments pretty useful and there's a problem with encoding '\t' in the JSON config file, so it would be nice if we could start using normal JS files for the configs.

{
    "requireCurlyBraces": [
        // All except "case" and "default".
        "if", "else", "for", "while", "do", "switch", "try", "catch"
    ],
    "requireSpaceBeforeKeywords": true,
    "requireSpaceAfterKeywords": [
        // All except "function".
        "do", "for", "if", "else", "switch", "case", "try", "catch", "void", "while", "with", "return", "typeof"
    ],
    "requireSpaceBeforeBlockStatements": true,
    "requireParenthesesAroundIIFE": true,
    "requireSpacesInConditionalExpression": {
        "afterTest": true,
        "beforeConsequent": true,
        "afterConsequent": true,
        "beforeAlternate": true
    },
    // Double check with "requireSpaceBeforeBlockStatements".
    // Covers FE, NFE and FD.
    "requireSpacesInFunction": {
        "beforeOpeningCurlyBrace": true
    },
    "disallowSpacesInFunction": {
        "beforeOpeningRoundBrace": true
    },
    "disallowSpacesInCallExpression": true,
    "requireMultipleVarDecl": true,
    "requireBlocksOnNewline": true,
    "requireSpacesInsideObjectBrackets": "all",
    "requireSpacesInsideArrayBrackets": "all",
    "requireSpacesInsideParentheses": "all",
    "disallowSpaceAfterObjectKeys": true,
    "requireSpaceBeforeObjectValues": true,
    "requireCommaBeforeLineBreak": true,
    "requireOperatorBeforeLineBreak": true,
    "disallowSpaceAfterPrefixUnaryOperators": true,
    "disallowSpaceBeforePostfixUnaryOperators": true,
    "requireSpaceBeforeBinaryOperators": true,
    "requireSpaceAfterBinaryOperators": true,
    "disallowKeywords": [
        "with"
    ],
    "disallowMultipleLineStrings": true,
    "disallowMixedSpacesAndTabs": true,
    "disallowTrailingWhitespace": true,
    // Will be introduced in 1.9.
    // "requireLineBreakAfterVariableAssignment": true,
    // TODO We haven't decided yet.
    "maximumLineLength": 140,
    "requireDotNotation": true,
    "disallowYodaConditions": true,
    // This is causing problems when JSCS's config is read from the .jscsrc file, but hopefully
    // it's going to work in config kept in a JS file.
    // The exception exists to allow including tab indented code in the comments which
    // may be a pretty common case.
    "requireSpaceAfterLineComment": {
        "allExcept": "\t"
    }
    "disallowNewlineBeforeBlockStatements": true,
    "validateLineBreaks": "LF",
    "validateQuoteMarks": "'",
    "validateIndentation": "\t",
    // Will be introduced in 1.9.
    // "requireSpaceBetweenArguments": true
}

Options to be considered for the boilerplate

• disallowPaddingNewlinesInBlocks - I would choose this option because I noticed that some of us add the padding and some don't,
• requirePaddingNewlinesBeforeKeywords - I would choose all except "typeof" and "else",
• disallowQuotedKeysInObjects - "allButReserved" I'm not sure about this one because in some cases it may lead to mixed quotes usage in one object. See e.g. https://github.com/ckeditor/ckeditor-dev/blob/master/core/loader.js,
• disallowImplicitTypeConversion,
• requireCamelCaseOrUpperCaseIdentifiers,
• disallowMultipleLineBreaks - I don't feel we need them - if some code should be visually separated then I prefer to that with a // ---- (up to 80 col) ---- comment,
• disallowKeywordsOnNewLine vs requireKeywordsOnNewLine - it affects were we can place comments. If we were making the decision independently of CKE4 and the code style I would say that we should go with requireKeywordsOnNewLine which causes least problems with comments (comment can be placed between } and else). However, you preferred keeping keywords in the same line, so now we can choose:
  • disallowKeywordsOnNewLine, but we'll need to keep comments in the previous block like I proposed in https://github.com/ckeditor/ckeditor5-design/issues/5#issuecomment-58168374 and this can be against indentation validator for blocks... so we may be in troubles soon,
  • none of the options and then we can place comments between } and else and (if there's no comment between) we can place the keyword in the same line as closing bracket; the downside is that it won't be checked by JSCS, but I prefer this option because it seems to be safer.
  • Edit: we can of course set disallowKeywordsOnNewLine to all keywords except else to allow comments and not use requireKeywordsOnNewLine at all.
• requireCapitalizedConstructors,
• requireCapitalizedComments,
• safeContextKeyword - I would check only that.

  Options to be checked in the future

• https://github.com/jscs-dev/node-jscs/issues/806 - indentation validator will be reintroduced in JSCS 1.9

  Options I did not include for more or less obvious reasons

• disallowSpaceAfterKeywords - we could specify "function", but it's validated by "disallowSpacesInFunctionExpression" and there are FEs where there's no space.
• disallowSpaceBeforeKeywords,
• disalloweSpaceBeforeBlockStatements,
• disallowSpacesInConditionalExpression,
• (require|disallow)SpacesIn(Anonymous)?Function(Expression|Declaration) - handled by (require|disallow)SpacesInFunction,
• requireSpacesInCallExpression,
• disallowMultipleVarDecl,
• requirePaddingNewlinesInBlocks,
• requirePaddingNewLinesInObjects,
• disallowPaddingNewLinesInObjects,
• disallowPaddingNewlinesBeforeKeywords,
• disallowEmptyBlocks,
• disallowSpacesInsideObjectBrackets,
• disallowSpacesInsideArrayBrackets,
• disallowSpacesInsideParentheses,
• disallowDanglingUnderscores,
• requireSpaceAfterObjectKeys,
• disallowSpaceBeforeObjectValues,
• disallowCommaBeforeLineBreak,
• requireAlignedObjectValues,
• requireSpaceAfterPrefixUnaryOperators,
• requireSpaceBeforePostfixUnaryOperators,
• disallowSpaceAfterBinaryOperators,
• disallowSpaceBeforeBinaryOperators,
• disallowOperatorBeforeLineBreak,
• disallowTrailingComma,
• requireTrailingComma,
• requireLineFeedAtFileEnd,
• requireYodaConditions,
• disallowSpaceAfterLineComment,
• disallowAnonymousFunctions,
• requireAnonymousFunctions,
• disallowFunctionDeclarations,
• requireFunctionDeclarations,
• requireNewlineBeforeBlockStatements,
• disallowSpaceBetweenArguments,
• disallowCapitalizedComments,

[fredck]

it would be nice if we could start using normal JS files for the configs

They where inside the dev/tasks/jscs.js earlier, but being JSCS (and JSHint) very popular tools, it is possible to integrate their standard json configuration files into IDEs (e.g. WebStorm) so the IDE will perform jscs checks while editing.

[Reinmar]

PS. I added third option for the comments problem (look for "Edit").

[Reinmar]

it would be nice if we could start using normal JS files for the configs They where inside the dev/tasks/jscs.js earlier, but being JSCS (and JSHint) very popular tools, it is possible to integrate their standard json configuration files into IDEs (e.g. WebStorm) so the IDE will perform jscs checks while editing.

Makes sense. So we'll have this topic as a comment :).

[fredck]

Proposed config

I've never been a fun of requireMultipleVarDecl. There are occasions that, for readability, it is better to have several var. For example when setting vars to complex objects or anything that takes more than one line. Or when adding comments in between vars. Or when having lots of vars for different groups of things.

As for the rest, I'm totally fine.

[fredck]

disallowPaddingNewlinesInBlocks - I would choose this option because I noticed that some of us add the padding and some don't,

I agree.

requirePaddingNewlinesBeforeKeywords - I would choose all except "typeof" and "else",

I'm sure there will be WTF cases here, so I would not be too strict.

disallowQuotedKeysInObjects - "allButReserved" I'm not sure about this one because in some cases it may lead to mixed quotes usage in one object. See e.g. https://github.com/ckeditor/ckeditor-dev/blob/master/core/loader.js,

I agree. We don't have to be too strict here, but there could be an internal convention to avoid them when possible.

disallowImplicitTypeConversion,

Agree, except for Boolean.

requireCamelCaseOrUpperCaseIdentifiers,

Should be ok. We can drop it later if not.

disallowMultipleLineBreaks - I don't feel we need them - if some code should be visually separated then I prefer to that with a // ---- (up to 80 col) ---- comment,

Agree.

disallowKeywordsOnNewLine vs requireKeywordsOnNewLine - it affects were we can place comments. If we were making the decision independently of CKE4 and the code style I would say that we should go with requireKeywordsOnNewLine which causes least problems with comments (comment can be placed between } and else). However, you preferred keeping keywords in the same line, so now we can choose: disallowKeywordsOnNewLine, but we'll need to keep comments in the previous block like I proposed in ckeditor/ckeditor5-design#5 (comment) and this can be against indentation validator for blocks... so we may be in troubles soon,

I like requireKeywordsOnNewLine as well, but this is controversial.

none of the options and then we can place comments between } and else and (if there's no comment between) we can place the keyword in the same line as closing bracket; the downside is that it won't be checked by JSCS, but I prefer this option because it seems to be safer.

Do you mean this?

if ( a ) {
}
// My else comment.
else if ( b ) {
}

if ( c ) {
} else if ( d ){
}

requireCapitalizedConstructors,

Agree.

requireCapitalizedComments,

Hum... WTFs in the horizon. It may be a convention. E.g. // jQuery whatever.

safeContextKeyword - I would check only that.

Agree.

[mikesherov]

Just a quick note about jscs-dev/node-jscs#806, indentation still is validated for blocks, but indentation for arrays and objects will be a separate rule introduced in 1.9

[Reinmar]

Just a quick note about jscs-dev/node-jscs#806, indentation still is validated for blocks, but indentation for arrays and objects will be a separate rule introduced in 1.9

Thanks for clarification. I haven't seen any indentation errors for a while, so I assumed that it was totally disabled. It turns out that our code is so perfect :D.

Anyways, we'll need to check if the new options will be ok for us, because the pre JSCS 1.7 behaviour was too strict.

[fredck]

I've never been a fun of requireMultipleVarDecl. There are occasions that, for readability, it is better to have several var. For example when setting vars to complex objects or anything that takes more than one line. Or when adding comments in between vars. Or when having lots of vars for different groups of things.

In fact, the boilerplate code fails hard on this. E.g.: https://github.com/ckeditor/ckeditor-boilerplate/blob/master/dev/tasks/utils/tools.js#L15-L27

[Reinmar]

I've never been a fun of requireMultipleVarDecl.

I'm personally ok with both - merging subsequent declarations or not merging at all. But I don't like a total mix where some declarations are merged and some short ones, which I would normally merge, someone left not merged for no good reasons. Therefore, I would then enable the disallowMultipleVarDecl. Especially that in CKE4 we merge declarations, so many mistakes will be made when switching between projects.

[Reinmar]

requirePaddingNewlinesBeforeKeywords - I would choose all except "typeof" and "else",

I'm sure there will be WTF cases here, so I would not be too strict.

If we limit this to typical control statements and loops like if, for, while, do, try, then I think it will be fine. These statements cannot be used as expressions in any case, so there should be no WTFs :).

I agree. We don't have to be too strict here, but there could be an internal convention to avoid them when possible.

OK.

Agree, except for Boolean.

OK.

Do you mean this?

Exactly.

Hum... WTFs in the horizon. It may be a convention. E.g. // jQuery whatever.

Huh... I forgot about such cases :D So yeah - it doesn't make sense.

[Reinmar]

I'm personally ok with both - merging subsequent declarations or not merging at all. But I don't like a total mix where some declarations are merged and some short ones, which I would normally merge, someone left not merged for no good reasons. Therefore, I would then enable the disallowMultipleVarDecl. Especially that in CKE4 we merge declarations, so many mistakes will be made when switching between projects.

Hm... There's one thing I don't line in not merging at all about which I forgot - declarations without assignments:

var x;
var y;
var z;
var foo;

And there's an option for that - disallowMultipleVarDecl: 'exceptUndefined'. It would be perfect.

[fredck]

And there's an option for that - disallowMultipleVarDecl: 'exceptUndefined'. It would be perfect.

I don't think this is good. It seems to enforce always having multiple var declarations if there is assignment, which is not good as well.

I think this case is not a big deal and we can have this as an internal convention instead of an enforced jscs rule.

[fredck]

requirePaddingNewlinesBeforeKeywords - I would choose all except "typeof" and "else",

I'm sure there will be WTF cases here, so I would not be too strict.

If we limit this to typical control statements and loops like if, for, while, do, try, then I think it will be fine. These statements cannot be used as expressions in any case, so there should be no WTFs :).

We may give it a try and see. Btw, I assume this doesn't apply to comments.

[fredck]

Do you mean this?

Exactly.

I'm totally fine for that.

[Reinmar]

And there's an option for that - disallowMultipleVarDecl: 'exceptUndefined'. It would be perfect.

I don't think this is good. It seems to enforce always having multiple var declarations if there is assignment, which is not good as well.

I think this case is not a big deal and we can have this as an internal convention instead of an enforced jscs rule.

The thing is that I already see that I would merge declarations in https://github.com/ckeditor/ckeditor-boilerplate/blob/master/dev/tasks/utils/tools.js#L15-L27 and you merged only some of them. Now, multiply this by number of developers and it's random.

Keeping consistency and internal rules is not easy and requires time when we have 2-4 devs working on one code. But we'll have more and then we then we can expect everything. I just know how much of my time these small details take when reviewing. My goal is to reduce the time we waste by having rules as precise as possible. I agree that forcing any of the option will induce some cases where code must be formatted slightly differently than what we would expect, but that's a cost of automation. And that's also why I'll accept any option if only applying it will not consume my (precious :D) time.

[Reinmar]

We may give it a try and see. Btw, I assume this doesn't apply to comments.

I hope so. If not then we can't use.

[fredck]

The thing is that I already see that I would merge declarations in https://github.com/ckeditor/ckeditor-boilerplate/blob/master/dev/tasks/utils/tools.js#L15-L27 and you merged only some of them. Now, multiply this by number of developers and it's random.

Sincerely speaking, I don't see any problem if it is merged in one part of the code and not in another. Both ways are good as long as the code is beautiful and readable, at the same time having a general convention that we tend to merge them.

[fredck]

Same here: https://github.com/ckeditor/ckeditor-boilerplate/blob/e4c9e85c1506960b5d497c9e1d73c71ea4c19eff/dev/tasks/utils/tools.js#L35-L44

[Reinmar]

Exactly - what was the reason to not merge those declarations in tools.js#L35-44? :D First I thought that it's because of the comment, but then I saw in line 23 that it's not the reason. And it starts like this - you lose confidence and starts wondering what rules those guys are following and how should I do that. Then you stop to care and you do whatever you like.

You know how pedantic I am and still, while working on Kuma where there are some rules applied to 75% of the code but no rules applied to the other 25%, after a while I stopped following many of them, because I just forgot what they are.

I read recently about very interesting study. Scientist were wondering why some buildings in one city suddenly turns into a ruin while others are perfect. It turned out that everything starts from one broken window or one graffiti which is not quickly mended and then people's nature is to accept that things are broken and brake them further. The same applies to code.

I'm not saying that merging or not merging makes a huge difference by itself, but that's just a one factors which may decrease code quality in general.

[fredck]

It is clear that the stuff in these lines of code are related, just like the ones in these. They're like two unrelated blocks of code. Added by the fact that the first one has a variable that spams to multiple lines.

Anyway. My point is that the one and only reason for code style is to make code beautiful and readable by humans, not beautiful and readable by machines. The machine should not stop me to make my code better. That's why I have as the first rule: "none of these rules apply if the goal is making the code better".

I know that you'll say that it helps making the code better instead, by avoiding having us making mistakes. Which is right and that's why we're adopting it for the great majority of our code style checks.

Still machines are not perfect because they don't think. We don't have to get that lazy and stop thinking. After all, the most important rule is "write good quality code, period".

Finally, we're talking about a case where one way or the other don't change the code quality.

[fredck]

Anyway, satisfying me or you is not the point. Others should step forward and give their opinion. One of us will just have to accept what the majority decides in this case. We'll incorporate it and move ahead, without further discussion.

[Reinmar]

Anyway, satisfying me or you is not the point. Others should step forward and give their opinion. One of us will just have to accept what the majority decides in this case. We'll incorporate it and move ahead, without further discussion.

Definitely :D That's why we keep bouncing up this thread in their mailboxes :D.

[jodator]

For me https://github.com/ckeditor/ckeditor-boilerplate/blob/e4c9e85c1506960b5d497c9e1d73c71ea4c19eff/dev/tasks/utils/tools.js#L35-L44 look weird. I'd rather have one var or multiple vars. Above link looks like merge conflict fix without code style applied ;-)

I tend to this option (more readable(==beautiful) in my opinion):

var task, taskConfig, config, all,i sAll;

task = options.task;
taskConfig = {};
config = {
    options: options.defaultOptions
};
taskConfig[ task ] = config;

// "all" is the default target to be used if others are not to be run.
all = options.targets.all;
isAll = true;

In my personal view it is more readable.

Also multiple assignments in above link might get lost (I've discovered that after 4th look on that piece of code).

[jodator]

Also giving clear advice on vars is better since I don't have to think "to merge or not to merge" since @fredck idea about merging "semantically" is well too opinion based. One might thinkt in that manner other one in different but when you have clear guidelines: "only one var per scope" it is trival to follow.

[oleq]

Because of

It turned out that everything starts from one broken window or one graffiti which is not quickly mended and then people's nature is to accept that things are broken and brake them further. The same applies to code.

I'm for merged declarations though I used to split them in many places. You can always place a new line or a comment between declarations to separate it, right?

The only case that makes me feel uncomfortable is a function expression, i.e. https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/lineutils/plugin.js#L74-L95 It just looks strange if declared next to "plain variables", not in a separate "var" block.

[mlewand]

IMHO the most readable solution is to splitting to multiple var statements no matter what.

Additionally it makes finding variables so much easier, just by marking them with var keyword. That gives two benefits:

• Differentiates variable assignment from the logic code.
• It let you very easily lookup variable name if you forgot it without using code completion (eg. you're browsing, and you don't have a caret in this scope that you're looking at).

It's super clear.

There is one thing, good programming books often recommends to put all the variable declarations at the very beginning of the method.

It's generally a good rule, but sometimes there case like this one here, or famous tmp vars. I think it's better to place them closer (still grouped) closer to the place of usage.

• 1 For separated var initialization no mater what.

Hm... There's one thing I don't line in not merging at all about which I forgot - declarations without assignments:

It's still not that bad, maybe the thing is that you're not used to it? And still typically there is less uninited vars than the ones with initialization.

@jodator

This idea with merging variable in single line is bad for changes, and it's hard to find the names there.

I don't have to think "to merge or not to merge" since @fredck idea about merging "semantically" is well too opinion based. One might thinkt in that manner other one in different but when you have clear guidelines: "only one var per scope" it is trival to follow.

Above statement is actually super important. Otherwise it would be good R- window, which then might result with a response that implmeneter has still different opinion on that. That would only result with skype message "do you think that...".

[jodator]

@mlewand so you propose:

var task = options.task;
var taskConfig = {};
var config = {
    options: options.defaultOptions
};
var taskConfig[ task ] = config;

// "all" is the default target to be used if others are not to be run.
var all = options.targets.all;
var isAll = true;

If yes, I'm ok with this also (keep in mind that js minifier would marge that vars anyway). If no, give example as reading long text is hard.

[mlewand]

@jodator Holly cow, I forgot to paste the code listing! My code was exactly the same as you gave. So you got the point.

var task = options.task;
var taskConfig = {};
var config = {
    options: options.defaultOptions
};

taskConfig[ task ] = config;

// "all" is the default target to be used if others are not to be run.
var all = options.targets.all;
var isAll = true;

Here I just removed var from taskConfig[ task ], because it's already defined. From your initial comment I've felt that you don't like multiple assignments in a single var statement - same for me ( ;

And as of minifier, ofc we don't care what it's working with, as long as it works.

[jodator]

Here I just removed var from taskConfig[ task ], because it's already defined.

There I was just copying & pasting mindlessly so count that as a typo.

[pjasiun]

I like separate vars with the exceptional for undefined. It would be good for diff/merging, because last ";" will not change into "," every time you add var, only one line will change instead of 2.

So I'm for:

var task = options.task;
var taskConfig = {};
var config = {
    options: options.defaultOptions
};

taskConfig[ task ] = config;

// "all" is the default target to be used if others are not to be run.
var all = options.targets.all;
var isAll = true;
var i, j, k;

[Reinmar]

Ok, so for now one thing can be noticed - opinion-based, mixed solution is not preferred by you.

Another two interesting points are:

• function expressions used in variable declarations are ugly (and I agree with that),
• defining all variables and then assigning values (@jodator's preference) is how the JS language works.

As for the first point I wanted to propose that function expressions can be used only when passed to functions and in all other cases function declarations should be used. The question that remains is whether function declarations should be placed at the end or at the beginning of the scope. I'm for the end, what makes the outer function more readable. (And if someone wants to raise the argument that sometimes they should be close to their usage places, the answer will be - it means that your outer function is too long :D.)

As for variable declarations - I see that separate declarations are leading. I also think that PJ's favourite version, which can be covered by disallowMultipleVarDecl: 'exceptUndefined' is a good middle ground. Finally, the arguments for separate declarations are valid:

• better diffs,
• no problem with changing comments indentation when new variable was added:

// This is "foo".
var foo = 1;

    // This is "foo".
var foo = 1,
    // This is "bar".
    bar = 2;

// vs...

// This is "foo".
var foo = 1;
// This is "bar".
var bar = 2;

[pjasiun]

The question that remains is whether function declarations should be placed at the end or at the beginning of the scope. I'm for the end, what makes the outer function more readable.

Definitely end. General code first (main function), specific code later (nested functions).

[jodator]

+1 for inner functioon at the end of scope as @pjasiun wrote.

[Reinmar]

Here goes our little pedantic baby: https://github.com/ckeditor/ckeditor-boilerplate/commit/2108275ff643effcd394b6d4782de4f4921498dd#diff-244785c57757a1d35f51e6091cc966fbR24

[jodator]

Does this JSCS config will be used in CKEditor-v5 as well? I'm asking in terms of adopting the same hint/code style options also in other projects?

[fredck]

Yes, we're supposed to spread this everywhere.

[Reinmar]

Yes, we're supposed to spread this everywhere.

Except CKE4 and I think that, for consistency, all its plugins (kept in separate repos).