-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Update: add latestEcmaVersion & supportedEcmaVersions #430
Conversation
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.
A couple of small things I noticed but no blockers.
lib/options.js
Outdated
* Get the latest ECMAScript version supported by Espree. | ||
* @returns {number} The latest ECMAScript version. | ||
*/ | ||
function latestEcmaVersion() { |
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.
I’d suggest calling this getLatestEcmaVersion to indicate a value is being retrieved (verb as opposed to noun).
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.
Good call! I was initially thinking of it being more of a static property (like version
), but that makes more sense with this iteration 👍
lib/options.js
Outdated
* Get the list of ECMAScript versions supported by Espree. | ||
* @returns {number[]} An array containing the supported ECMAScript versions. | ||
*/ | ||
function supportedEcmaVersions() { |
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.
Similarly, getSupportedEcmaVersions.
lib/options.js
Outdated
* @returns {number} The latest ECMAScript version. | ||
*/ | ||
function latestEcmaVersion() { | ||
return Math.max(...SUPPORTED_VERSIONS); |
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.
You can avoid a calculation here by using SUPPORTED_VERSIONS[SUPPORTED_VERSIONS.length - 1]
Thanks for the review! Updated. |
Ah, I like that! That’s more in line with my initial design and I think is a nicer API. Will update. |
1439ee6
to
270da9c
Compare
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.
Yes! Very nice now.
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.
LGTM. I can see this being useful, and I like how tiny the API addition ends up being.
Inspired by this discussion, this is a proposal to add
latestEcmaVersion()
&supportedEcmaVersions()
to the public API.Benefits:
ecmaVersion
from the parser for rules (as is mentioned in the comment listed above).ecmaVersion
s in our demo have gotten out of sync with ESLint core. This would allow us to calculate this programmatically and not have to manually update it.Cons:
Open questions: