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

Conversation

Projects
None yet
9 participants
@ilyavolodin
Copy link
Member

ilyavolodin commented Nov 28, 2016

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

This comment has been minimized.

Copy link

eslintbot commented Nov 28, 2016

LGTM

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Nov 28, 2016

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

@platinumazure
Copy link
Member

platinumazure left a comment

Just leaving some preliminary, basic comments/suggestions.

const version = eslintCLI.version();

assert.isString(version);
assert.isTrue(parseInt(version[0], 10) >= 3);

This comment has been minimized.

@platinumazure

platinumazure Nov 28, 2016

Member

Is there any harm to just getting the package version and comparing more strictly?

This comment has been minimized.

@ilyavolodin

ilyavolodin Nov 28, 2016

Member

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);

This comment has been minimized.

@platinumazure

platinumazure Nov 28, 2016

Member

Is there any harm to just getting the package version and comparing more strictly?

* @returns {Object} All loaded rules
*/
function getAllLoadedRules() {
const allRules = {};

This comment has been minimized.

@nzakas

nzakas Nov 28, 2016

Member

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

This comment has been minimized.

Copy link

eslintbot commented Nov 28, 2016

LGTM

});
});

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

This comment has been minimized.

@kaicataldo

kaicataldo Nov 29, 2016

Member

Looks like we might be missing an it here

* Gets current version of ESLint
* @returns {string} version number
*/
CLIEngine.version = function() {

This comment has been minimized.

@nzakas

nzakas Dec 1, 2016

Member

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() {

This comment has been minimized.

@nzakas

nzakas Dec 1, 2016

Member

Same comment, I'd just make it a value rather than a function.

@ilyavolodin ilyavolodin force-pushed the issue6525 branch from e5ad2f1 to 655a3cf Dec 3, 2016

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 3, 2016

LGTM

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Dec 4, 2016

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 4, 2016

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

This comment has been minimized.

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.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 8, 2016

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

@kaicataldo
Copy link
Member

kaicataldo left a comment

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"),

This comment has been minimized.

@kaicataldo

kaicataldo Dec 8, 2016

Member

Nit: extra newline

This comment has been minimized.

@ilyavolodin

ilyavolodin Dec 9, 2016

Member

That was intentional. Following the style of

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

This comment has been minimized.

@kaicataldo

kaicataldo Dec 9, 2016

Member

Thanks for the explanation

@platinumazure
Copy link
Member

platinumazure left a comment

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

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 9, 2016

@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

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 9, 2016

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.

All fixed

@ilyavolodin ilyavolodin merged commit 2cdfb4e into master Dec 9, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@ilyavolodin ilyavolodin deleted the issue6525 branch Dec 9, 2016

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Dec 10, 2016

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

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 10, 2016

@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 eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.