-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
esversion
option
#2519
esversion
option
#2519
Conversation
@@ -4247,12 +4305,12 @@ var JSHINT = (function() { | |||
x.lbp = 25; | |||
}(prefix("yield", function() { | |||
var prev = state.tokens.prev; | |||
if (state.inESNext(true) && !state.funct["(generator)"]) { | |||
if (state.inES6(true) && !state.funct["(generator)"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generator functions aren't in 6.. but I guess the plan for this is for v2.x.. meaning it cannot break?
Still might be good to differentiate now e.g. esnext means esversion = 7 and this requires es7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generator functions aren't in 6
Do you mean generator comprehensions?
Still might be good to differentiate now e.g. esnext means esversion = 7 and this requires es7?
If you mean the esnext option, it would be a breaking change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, im doubting myself now, but anyway the gyst is...
Any es7 features should only be on in esversion = 7 and esnext should map to esversion 7. That way esversion is "correct" and it is not a breaking change..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, generators did make es6, this thread only applies to arrray comprehensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but I've not yet documented esversion: 7
becasue of #1939 (comment)
Not all the comments are for you, some are just things that probably need "deciding" by someone (so probably @jugglinmike). I wouldn't do anything based on them yet till he responds and its too risky a changeset for me to consider merging, so I'll be leaving it to him. |
@@ -2589,11 +2647,11 @@ var JSHINT = (function() { | |||
prefix("[", function() { | |||
var blocktype = lookupBlockType(); | |||
if (blocktype.isCompArray) { | |||
if (!state.inESNext()) { | |||
if (!state.inES6()) { | |||
warning("W119", state.tokens.curr, "array comprehension"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, not ES6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
We're going to keep (Moving forward, we will remove list comprehensions from ES6 but persist them in "moz".) |
@jugglinmike See #1148 (comment). Since "moz" isn't standard ECMAScript, I think it shouldn't be included in Another question: can |
I'm fine with this, but by the same token,
No. You can re-use the logic I introduced for the |
I dont like the moz option. We should remove let blocks since afaik they are a dead proposal, replaced by let proper and not have moz. Array comprehensions should go under 7 or esversion expeimental since afaik they are on the cards for 7/es2016. we could say it is not a breaking change for experimental to evolve..
|
Removing
It would be a bug not evolving following proposals, but implementing an older version of them 😉
I think (NOTE: Making |
I'm not a fan of it either, but it's a commitment that was made for us, and we should honor it so long as it doesn't impede continued development. I have no idea what the active usage is like, but a simple GitHub code search suggests that removing it would put a lot of people off.
It's not about intuitiveness, it's about correctness. ES7 doesn't exist yet, and when it does, it will be a moving target. It should be considered experimental and exposed in a way that makes this clear. Definitely out-of-scope for this patch, though. |
a83c622
to
db91f99
Compare
I've removed
I agree with you |
Before last commit, linting this code: // jshint esversion: 3
// jshint moz: true
var a = {
get x() {}
}; JSHint warned that getters are es5 features, even if moz is > than ES5 |
I remember seeing plenty of bugs about people not realising they had to have moz and es5/esnext on, but I think they were mostly in reference to a previous breaking change. |
Should tests use |
I think so.. it needs to be done at some point anyway. It seems like alot of hassle so you might want to wait for @jugglinmike's opinion. |
I'd rather not since it adds a lot of noise to the non-trivial change happening here. I think that change would fit better in a single "Remove version-specific language options" commit, somewhere down the line |
@nicolo-ribaudo you might be interested to see Mike's blog post on jshint's way forward w.r.t. esversion - http://jshint.com/blog/new-lang-features/ |
I think I can revert
Thanks for the link |
Rebased on |
1205c91
to
0ed24c5
Compare
@nicolo-ribaudo This patch no longer merges cleanly, but it doesn't seem fair to ask you to rebase again. I've squashed the commits together and added a single merge commit to resolve the conflicts; you can find that on the If that looks good to you, I can either squash those commits together or land them as-is. Or, if you prefer, you can of course resolve the conflicts yourself. Please let me know how you'd like to proceed. I'm sorry I let this go for so long! |
@jugglinmike I won't be able to rebase this until next weekend. |
This option (which may be set to 3, 5, or 6 / 2015) should replace `es3`, `es5` and `esnext`. This: - precludes invalid configurations like `es3: false, es5: true` - scales better for future versions of ECMAScript - eliminates the ambiguity of `esnext` (whose meaning has changed now that ES6 has been finalized) This commit partially fixes jshint#2124
Now "esnext" means es7
0ed24c5
to
22bbf39
Compare
22bbf39
to
4f1eeb9
Compare
@jugglinmike Rebased, to not introduce noise (the merge commit) in commits history. |
A man after my own heart. Squashed and merged to |
…ne `esnext` This commit fixes a regression introduced in jshintgh-2519
…ne `esnext` This commit fixes a regression introduced in gh-2519
…ne `esnext` This commit fixes a regression introduced in jshintgh-2519
No description provided.