requireSemicolon implementation

[lahmatiy]

Boolean rule that requires semicolon after:

• var declaration
• expression statement
• return
• throw
• break
• continue
• do-while

Valid:

var a = 1;
var fn = function(){ return; };
if (a > 0)
  b = a * 5;

Invalid:

var a = 1
var fn = function(){ return };
if (a > 0)
  b = a * 5

[markelog]

You have problems with indentation, could also update readme file?

[lahmatiy]

Fixed

[lahmatiy]

Looks much simpler for now.
I'm not sure about search performance. Will need to use binary search for token lookup in future.

[mdevils]

cc @markelog

[lahmatiy]

Any news?

[markelog]

I think one example is enough for readme, rule is pretty much self-descriptive.

How this corresponds with asi option of JSHint? it's not being dropped as far as i know.

The opposite rule might be helpful though, but if JSCS would have disallowSemicolon rule, it's better to have requireSemicolon too. So i would land this if we would have both, but this open for discussion.

@mikesherov, @mdevils?

[lahmatiy]

jscs behave like JSHint with asi option now.

disallowSemicolon is more complicated than requireSemicolon. We can't warn on every semicolon, in some cases it still required:

• function declaration or expression and following expression in brackets
• sequence of IIFE
• in the begining and/or in the end of file to avoid previous problems on file concatenation

maybe something else...

[markelog]

Don't warn on every semicolon, just on ones that you described in the PR description, maybe we would need to change the name of the rule, but i fine with the current one, but perhaps explanation of it should be more verbose.

[lahmatiy]

That's not enough, for example:

var foo = function(){};   // use semicolon
(123)
// foo -> [function]

var foo = function(){}   // don't use semicolon
(123)
// foo -> undefined

[markelog]

Opposite rule should do only opposite checks, this is why i noted –

maybe we would need to change the name of the rule, but i fine with the current one, but perhaps explanation of it should be more verbose

But this could go another way too, i'm just considering options here, since esprima AST is verbose enough to catch cases you mentioned above. But only way i see to land this is to create the opposite rule.

[markelog]

Perhaps we could reconsider this, if the opposite rule would be presented

[lahmatiy]

Strange decision.
I believe features (rules) should be implemented one by one. I'm not sure that I can implement opposite rule properly, and also I have no time for that now. Opposite rule isn't just comparison operator invertion, it's more complicated as I mentioned above.
But requireSemicolon rule would be useful for many developers right now. Maybe somebody else implement opposite rule later. There are many rules in jscs with no opposite rule, and I think it's ok.
cc @mdevils

[markelog]

I closed it because it's a duplicative rule of jshint asi option, we recommend use jscs with jshint and that's why we usually don't do rules like this.

The only way i can see to land this, is presented it with conjunction with opposite one, but i already said that.

[mdevils]

For now we don't duplicate jshint options. We will notify you in case if it will be changed. Thank you for you interest. I'm looking forward for new pull-requests from you covering other cases.

[lahmatiy]

jshint requires semicolon by default, asi is relaxing option.
Semicolon at the end of expressions is code style related thing. JSCS checks code style. That's really strange that I should use other tools with JSCS to check my code style completely...

[mdevils]

Good point. We will discuss it. Keep in touch.

[pickhardt]

+1 for getting this into JSCS.

JSHint version 3 is planning to remove all style warnings:
http://www.jshint.com/blog/jshint-3-plans/
"Remove all style-related options and warnings. If it makes sense they should be moved into optional plugins."

They are planning the following:
"JSCS enforces style conventions e.g. whether to put a space in between function keyword and a parenthesis or not. JSHint looks for potential problems and bugs: using undefined variables, leaking variables into the global scope, using constructs that don't work in all environments and so on."

It's unclear to me whether JSHint will still require semicolons or not. Since there's automatic semicolon insertion, it isn't technically a bug.

I'm probably a typical user of JSHint and/or JSCS and/or Closure... we just want one tool and one config file to maintain. The goal is to get a good code quality tool to enforce quality code. It doesn't matter whether it is a bug or bad style, what matters is code quality, which is inclusive of both.

[mikesherov]

Whether or not ASI is style or not is debatable, however, JSCS has no current plans to implement gotcha checks (like accidental globals, unused vars, typos...).

If you're looking for one tool, JSCS isn't it currently.

Our focus is an unopinionated, composable, configurable style guide enforcer. JSHint does a great job finding REAL bugs. JSCS is currently here to make your code pretty. We believe a focus or style will enable us to do it well.

[pickhardt]

I get that it's your decision (and JSHint's) to focus on different topics. It's just going to less useful than a combined project would be. For instance, is a missing semicolon bad style, a bug, both, or neither? There's almost definitely going to be cases where JSCS says something's a bug and out of scope, and JSHint says something's a style and out of scope, and as a result neither will cover it.

In any case, the real issue for this PR is whether or not to include semicolons. I hope you guys do.

[edwardsmit]

As JSHint removed Style-Checking, our team (and we're not the only ones I suppose) are looking for an alternative because "All code in any code-base should look like a single person typed it, no matter how many people contributed." (https://github.com/rwaldron/idiomatic.js/)
Therefore I would really like to upvote this rule.

[mikesherov]

@edwardsmit @pickhardt if JSHint removes the ASI checks, we'll gladly implement it. I'm sure they havent' yet, and don't believe they are as per https://github.com/jshint/jshint/blob/2.x/dist/jshint.js#L52823

[adius]

+1. Totally support @pickhardt on this topic. I think requireSemicolon and disallowSemicolon are an absolute must for jscs. Actually that's the main reason I was checking out jshint, jslint and jscs in the first place, as the use of semicolons is really inconsistent in a project I'm currently working on. We eventually decided to go for a no-semicolon policy.
…and now neither of the tools supports it. Great. -.-

[mikesherov]

@adius, what makes you think JSHint is dropping semicolon detection?

[mikesherov]

Please refer to JSHint's docs, and the source code: https://github.com/jshint/jshint/blob/4ec82974b08808a976b4a263a52bb1bcc86debff/src/options.js#L5

I assure you JSHint is still doing semicolons, unless I missed an announcement somewhere.

[adius]

JSHint only supports "automatic semicolon insertion should be tolerated" but I want "disallowSemicolon" which translates to "semicolons must not be used unless they are absolutely necessary".

[mikesherov]

Neither tool ever supported it. This is true. I think it's safe for us to tackle this one unless Anton would consider a "don't use semicolons unless necessary rule". He has more of the machinery in place. If not, I think it's safe for us to handle it. @valueof could you weigh in on this:

1. JSHint has a "require semicolons" rule, a "allow no semicolons" rule, but lacks a "require no semicolons unless necessary" rule. Do you plan on ever implementing that rule?
2. If this answer to 1. is no, we'll likely provide that rule, but we'll also likely duplicate the existing rules from JSHint, for symmetry's sake.

Thoughts?

[kaleb]

I am evaluating node-jscs to use in my development processes. Although I prefer using semicolons, let's pretend that I hate them. It seems like it can be a code-style issue. In twbs/bootstrap#3057, for example, there are many reasons against using semicolons (mainly for stylistic reasons) and other reasons for using semicolons.

If the following is parsed with jshint:

function hi(name) {
  console.log('Hello, %s!', name);
}
hi('World')

There is one warning: "4: missing semicolon". But if I don't like semicolons, and I want my code style to have them as little as possible, I can tell jshint that I would like some ASI:

/* jshint asi: true */
function hi(name) {
  console.log('Hello, %s!', name);
}
hi('World')

That's all fine and dandy, but I don't want any unnecessary semicolons. I want something to tell me that there is an unnecessary semicolon on line 3. This desire of mine is a stylistic desire.

But we need to be careful. Because, let's say that I want to also say hello to a couple of awesome JS projects:

/* jshint asi: true */
function hi(name) {
  console.log('Hello, %s!', name);
}
hi('World')
['jscs', 'jshint'].forEach(hi)

jshint now correctly tells me that this is wrong with the following:

Seven warnings

• 6 ['jscs'] is better written in dot notation.
• 6 Expected ']' to match '[' from line 6 and instead saw ','.
• 6 Expected an assignment or function call and instead saw an expression.
• 6 Expected an assignment or function call and instead saw an expression.
• 6 Expected an identifier and instead saw ']'.
• 6 Expected an operator and instead saw '.'.
• 6 Expected an assignment or function call and instead saw an expression.

One undefined variable

• 6 forEach

It would also be nice if I were to have a tool that told me that I also had an unnecessary semicolon on line 3.

I however prefer to use semicolons, though, so I personally will never run into this problem.

[valueof]

JSHint—as of 2.5.0—now warns when semicolon is omitted but may cause problems even when asi:true. Unfortunately, I had to remove some checks from the original patch because they generated false positives. I'm definitely open to more improvements though. ASI option should ideally be allow ASI but only when it's not breaking stuff.

[mikesherov]

@valueof, that makes sense, but still leaves the following question unanswered: is JSHint planning a "disallow semicolons except when required" option?

[valueof]

@mikesherov No.

[adius]

You better should. There are a lot of projects which have this style. They should be able to check documents for conformity as well!

[lahmatiy]

As disallowSemicolon has landed in 1.9 (#732) it would be great to land requireSemicolon too.
I already use this implementation all this time via additionalRules and it's works fine.
/cc @mdevils @markelog @mrjoelkemp @christophercliff

[markelog]

Yep, would you like to rebase this or open a new PR?

[lahmatiy]

Sorry for delay.
I've made a new PR #951