requireDotNotation: Require dots for es3 keywords when not in es3 mode.

[mikesherov]

No description provided.

[mikesherov]

/cc @mdevils, @markelog, @mrjoelkemp review please?

[mikesherov]

I made this an options hash because I plan on also making tabSize a file level option and didn't want to go adding more and more formal parameters.

[coveralls]

Coverage increased (+0.02%) when pulling cab9f35 on mikesherov:fileConfig3 into bcac436 on jscs-dev:master.

[mdevils]

Hello, Mike.

Good job on this PR. In this implementation I can see es3: true, es6: true values. What if we introduce es: 3, es: 6? This may be simpler?

[mikesherov]

@mdevils I toyed around with a single variable for which versions we need to target, and started off with your suggestion (I even had a getLanguageVersion function.

Are there any cases where multiple discrete values matter? If not, I can go back to a single value.

[mdevils]

Are there any cases where multiple discrete values matter?

I don't think so.

[mikesherov]

@mdevils consider the following code:

function() {
   'use strict';

   // with should fail ONLY in ES5 enviroments
   with (window) {
     getComputedStyle();
   }

  // should fail ONLY in es3 environments
  a.while = 2;
}

Should both a.while and with (window) produce errors simultaneously? Meaning, should you be able to specify that you're only targetting ES3? Or does ES3 also assume ES5?

If it's the latter, and we forgo the ability to target only es3, then yes, I can get away with es: 3. Otherwise, I'm stuck with boolean flags for each language version.

[mikesherov]

Nevermind, it's nonsensical to say my code ONLY supports IE6. I'll switch it to a number.

[mdevils]

@mikesherov, thank you 😉

[qfox]

Probably dialect: 'es3' will be better.

[mikesherov]

Just for the record, the only API I'm changing here is the one in JsFile, right @mdevils ?

[mikesherov]

@zxqfox in this case, version is more appropriate than dialect, no?

[qfox]

@mikesherov You mean version: 'es3'? I just wanna to say that es: 3 is much better than es3: true but we can check even actionscript with custom esprima. No?

[mdevils]

Mike, I have one more idea.

What if we introduce new class Language (or better Dialect) and implement methods getKeywords(), isKeyword(...), isReservedWord(...) in that class (having child classes Es3Dialect, Es5Dialect, for instance).

And then rules would just use it: if (file.getDialect().isKeyword(keyword)) { ... }.

So we completely encapsulate this in a class and in the future we can even introduce dialects like JSX and others without touching the rules.

[qfox]

I'm agreed with @mdevils. It's pretty nice to have something like that and I hope there's no troubles with implementing this ;-)

[mikesherov]

OK, So I'll introduce the dialect class then here.

[mikesherov]

Thanks for the input folks!

[qfox]

😄 Great 👍

[mdevils]

CLI options are OK for me currently, Mike. It's only about JsFile 😉.

[mikesherov]

@mdevils although, it isn't that while is not a keyword in es5, it's more like isValidPropertyName where the answer is pretty much true for everything in ES5 and above. Doesn't change the Dialect stuff much.

But basically it means that the default Dialect class is es5 and isValidPropertyName has the base logic.

What's cool though is now all the utils keyword stuff moves out in the Dialect files instead of the utils file.

[mdevils]

@mikesherov are you sure while is not a keyword in ES5?

I've seen it here: http://www.ecma-international.org/ecma-262/5.1/#sec-12.10

[mdevils]

it's more like isValidPropertyName where the answer is pretty much true for everything in ES5 and above.

I like isValidPropertyName. So we can also have isValidObjectLiteralKey 😄

[mikesherov]

@mikesherov are you sure while is not a keyword in ES5?

I've seen it here: http://www.ecma-international.org/ecma-262/5.1/#sec-12.10

It's not that it's not a keyword, it's that keywords are allowed as property names in ES5

[mdevils]

@mikesherov, oh, OK.

[markelog]

-1 from me, es3 environment is dying but this stuff complicates our cli interface and rules configuration.

Not that my "-1" is a blocker more just a note but i feel uneasy with it.

[mikesherov]

-1 from me, es3 environment is dying but this stuff complicates our cli interface and rules configuration.

It does not complicate rule configuration, as it's not a rule configuration option, but rather a file option. We will need the idea of file configuration for tabSize as well.

I think honestly that for the case of correctness, until ie8 is really dead, we need to consider es3.

[mdevils]

I agree with @mikesherov. Currently most websites respect ES3 due to IE8 JS implementation.

[markelog]

Yep, this understandable and regrettable at the same time

[mikesherov]

:-/ I'll try to keep the impact on the design of the code to a minimum. At least in 2.0 we'll make es3 off by default.

[markelog]

Is this ready?

[mikesherov]

Not yet. However, I'm now thinking we should not implement what @mdevils suggested. I'm afraid that introducing a dialect class/concept is overkill for this.

@mdevils, how would you feel if I just updated this back to getLanguageVersiom and forgo the dialect concept?

[mdevils]

Option -t looks strange. What if we just remove short variant: .option('--es3', 'validates code as es3')?

I believe --es3 is short enough.

[mikesherov]

fair enough, I'll do that then.

[mdevils]

I'm afraid that introducing a dialect class/concept is overkill for this.

I believe this is best for the future. Later we can introduce many JS-dialects support like ES7,..., JSX. Especially JSX as React.js becoming more and more popular, we can, later, add possibility to put dialects in plugins.

[mikesherov]

Yes, I agree we need to support different variants, however, it's going to be hard to predict which pieces of functionality are important to abstract here.

I believe using conditional logic for specific rules by querying what dialect you're using is going to be less overhead in the long run than introducing a class per dialect with explicit methods.

To use this as a concrete example, isValidPropertyName would need to be implemented in all dialects despite the need for it in only one rule.

I would consider it better to say if (file->dialect ===es3) than have to introduce that function for all future dialects.

In this case, I prefer conditional over inheritance and don't see ample proof that the long term will see benefits from introducing another set of classes.

Thoughts?

[mdevils]

OK, Mike, lets postpone this abstraction layer until we face considerable amount of rules with dialect-specific behaviour.

[mikesherov]

OK, Mike, lets postpone this abstraction layer until we face considerable amount of rules with dialect-specific behaviour.

Thanks for compromising! I'm sure I'll regret my suggestion in the future! I'll update this PR now.

[mikesherov]

@mdevils can you re-review?

[coveralls]

Coverage increased (+0.03%) when pulling 520bfcc on mikesherov:fileConfig3 into 5d7bf3e on jscs-dev:master.

[mrjoelkemp]

If I'm following the code paths properly, the es3 flag is implicitly true by default. Should the its for this describe be moved up a level since this is the default behavior?

[mikesherov]

es3 will be off by default in 2.0. I purposefully excluded it from the default behavior test so I don't have to change it in the future.

[mrjoelkemp]

Nice

[mrjoelkemp]

LGTM

[mdevils]

Made a final review. Everything looks cool. Good job!

[markelog]

Didn't review it, but i'm gonna merge it, since this is blocking #868