Skip to content
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

Merged
merged 2 commits into from Dec 9, 2016
Merged

New: Additional APIs (fixes #6256) #7669

merged 2 commits into from Dec 9, 2016

Conversation

ilyavolodin
Copy link
Member

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 to linter
  • version to linter
  • version to CLIEngine

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.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@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.

Copy link
Member

@platinumazure platinumazure left a 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);
Copy link
Member

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?

Copy link
Member Author

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);
Copy link
Member

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 = {};
Copy link
Member

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.

@eslintbot
Copy link

LGTM

});
});

describe("when loading all rules", () => {
Copy link
Member

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

nzakas
nzakas previously requested changes Dec 1, 2016
* Gets current version of ESLint
* @returns {string} version number
*/
CLIEngine.version = function() {
Copy link
Member

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() {
Copy link
Member

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.

@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member

I think it's useful for plugin rules to support multiple versions if it adds version property to RuleContext as well.
Actually, I wanted it for eslint-plugin-node :)

@ilyavolodin
Copy link
Member Author

ilyavolodin commented Dec 4, 2016

@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.

@mysticatea
Copy link
Member

mysticatea commented Dec 4, 2016

In fact, there are cases that a rule wants to know the version of ESLint.

Breaking Changes

If a plugin wants to support both versions 1 and 2, the rule needs to address breaking changes.
For example, eslint-plugin-node needed to address this scope analysis changes until it dropped support ESLint 1.x.

New Features

Plugins can improve the linting result of rules as using new features.
But the rule cannot distinguish whether the current version of ESLint is supporting the new feature or not.
For example, eslint-plugin-node wants to use the range of warnings feature, but it cannot use the feature currently to support eslint@<3.1.0.

@ilyavolodin
Copy link
Member Author

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.

@mysticatea
Copy link
Member

mysticatea commented Dec 4, 2016

Of course, if there is a way except versions to distinguish whether ESLint supports features or not, it's better.
But I have no idea (I'm not sure your example. The set property of escope exists in both 1 and 2).

This is just a voice from me as a plugin owner.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint labels Dec 8, 2016
@ilyavolodin
Copy link
Member Author

ilyavolodin commented Dec 8, 2016

@eslint/eslint-team Can this be reviewed and merged? Would really like to get it into tomorrow's release.

Copy link
Member

@kaicataldo kaicataldo left a 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"),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra newline

Copy link
Member Author

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

pkg = require("../package.json");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation

Copy link
Member

@platinumazure platinumazure left a 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.

@ilyavolodin
Copy link
Member Author

@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. getAllRules is exposed on the linter API object, however, custom rules (outside of the plugins) could only be loaded through CLIEngine API. I'm also not sure if we have any tests that access both APIs at the same time.

@platinumazure
Copy link
Member

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.

@ilyavolodin ilyavolodin merged commit 2cdfb4e into master Dec 9, 2016
@ilyavolodin ilyavolodin deleted the issue6525 branch December 9, 2016 14:02
@mysticatea
Copy link
Member

mysticatea commented Dec 10, 2016

I'm sorry for late.
I realized Node.js API page has not been updated.

@ilyavolodin
Copy link
Member Author

@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.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants