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
New: Additional APIs (fixes #6256) #7669
Conversation
LGTM |
@ilyavolodin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @mysticatea and @nzakas to be potential reviewers. |
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.
Just leaving some preliminary, basic comments/suggestions.
const version = eslintCLI.version(); | ||
|
||
assert.isString(version); | ||
assert.isTrue(parseInt(version[0], 10) >= 3); |
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.
Is there any harm to just getting the package version and comparing more strictly?
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.
There isn't, but there's also not a whole lot of point in doing that either.
const version = eslint.version(); | ||
|
||
assert.isString(version); | ||
assert.isTrue(parseInt(version[0], 10) >= 3); |
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.
Is there any harm to just getting the package version and comparing more strictly?
* @returns {Object} All loaded rules | ||
*/ | ||
function getAllLoadedRules() { | ||
const allRules = {}; |
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 like to use a Map
here instead of an object, as it's more indicative of what the return value actually is.
LGTM |
}); | ||
}); | ||
|
||
describe("when loading all rules", () => { |
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.
Looks like we might be missing an it
here
* Gets current version of ESLint | ||
* @returns {string} version number | ||
*/ | ||
CLIEngine.version = function() { |
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.
Why not just CLIEngine.version = pkg.version
? I'm not sure there's much value in making it a function.
* Gets current version of ESLint | ||
* @returns {string} version number | ||
*/ | ||
api.version = function() { |
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 comment, I'd just make it a value rather than a function.
e5ad2f1
to
655a3cf
Compare
LGTM |
I think it's useful for plugin rules to support multiple versions if it adds |
@mysticatea I think that goes against our policies of rules not knowing anything about other rules and core. Doesn't feel like a good idea to change that, honestly. |
In fact, there are cases that a rule wants to know the version of ESLint. Breaking ChangesIf a plugin wants to support both versions 1 and 2, the rule needs to address breaking changes. New FeaturesPlugins can improve the linting result of rules as using new features. |
I think those should just be checks inside plugin. For example: if (context.getScope().set) {
// version 2
} else {
// version 1
} I'm not sure that giving direct knowledge of version of the core to the rule is a good idea. |
Of course, if there is a way except versions to distinguish whether ESLint supports features or not, it's better. This is just a voice from me as a plugin owner. |
@eslint/eslint-team Can this be reviewed and merged? Would really like to get it into tomorrow's release. |
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. Thanks!
@@ -26,7 +26,10 @@ const assert = require("assert"), | |||
Traverser = require("./util/traverser"), | |||
RuleContext = require("./rule-context"), | |||
rules = require("./rules"), | |||
timing = require("./timing"); | |||
timing = require("./timing"), | |||
|
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.
Nit: extra newline
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.
That was intentional. Following the style of
Line 33 in 655a3cf
pkg = require("../package.json"); |
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.
Thanks for the explanation
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.
Question: Do we have a test case that covers getAllLoadedRules()
returning a custom non-plugin rule (i.e., a rule that would be loaded by a --rulesdir
switch on the command line)? If not, I'd love to see one. I didn't have a chance to take a look at the full file, hence the comment.
@platinumazure On one hand, that's a good point, there's no test like that. On the other - it would be pretty hard to create, and I'm not sure how valid it would be. |
We could probably release without that test given custom rules directories are semi-deprecated. Just wanted to raise the question. I don't mind merging this now and improving coverage in a separate issue. |
I'm sorry for late. |
@mysticatea This has been discussed on the last TSC meeting. We agreed that our NodeJS API page needs a lot of work (since it doesn't cover any of the functions exposed by the API object), and decided to work on it separately from this PR. |
What is the purpose of this pull request? (put an "X" next to item)
[x] Add something to the core
What changes did you make? (Give an overview)
Added 3 new API methods.
getRules
tolinter
version
tolinter
version
toCLIEngine
Is there anything you'd like reviewers to focus on?
There's one failing test in the eslint.js file. I honestly don't know how to fix it, since it's only a problem when testing all of the ESLint files. If testing
eslint.js
file by itself, everything passes. This is due to us never cleaning loaded rules (and not having any abilities to clean loaded rules from inside eslint.js file). Any suggestions are welcome. PR is also missing documentation changes. I'll add them once I figure out how to fix failing test.