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

Proposal for additional APIs #6525

Closed
ilyavolodin opened this issue Jun 24, 2016 · 28 comments
Closed

Proposal for additional APIs #6525

ilyavolodin opened this issue Jun 24, 2016 · 28 comments
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 enhancement This change enhances an existing feature of ESLint

Comments

@ilyavolodin
Copy link
Member

ilyavolodin commented Jun 24, 2016

I would like to propose additional APIs. I'm not sure which object they should be attached to, as they don't seem to fit any of the current ones we have. Maybe linter, since they are all global.

version

Should return current ESLint version. This is necessary to correctly invalidate session storage data in the online demo, and probably useful for tools as well

getRules()

Should return all currently loaded rules. If CLIEngine.addPlugin has been called prior to this API call, it should also return rules from plugin. Return should be an array of object with rule name, rule schema and current configuration.

getRuleMetadata(null|string)

Should return metadata for all currently loaded rules (plugins included). Can be merged with getRules command.

@ilyavolodin ilyavolodin added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 24, 2016
@nzakas
Copy link
Member

nzakas commented Jun 24, 2016

  • version should be on the ESLint object and probably mirrored on CLIEngine.
  • getRules - this proposal seems odd, I'm not a fan of the argument to return just plugin rules. What's the use case for that?
  • removePlugin - why is this needed? We don't remove plugins during a run.

@ilyavolodin
Copy link
Member Author

@nzakas both getRules(string) and removePlugin are for the same use-case. Ability to try out plugins in online demo, or dedicated tool. We don't remove plugins during a run, but Node API is not just about run, but also about setup (basically replacement for config file). With config file, I can always remove plugin by deleting it from plugins array, but in API there's no way to currently do that.
I'd be OK to remove string parameter from getRules, and have it always return all rules, it just felt like it might be easier in some cases to run getRules() first, then addPlugin and then getRules(plugin-name).

@platinumazure
Copy link
Member

Maybe for clarity, we could have getPluginRules(pluginName), getCoreRules() (maybe), and getAllRules()?

@nzakas
Copy link
Member

nzakas commented Jun 24, 2016

There's no need to remove plugins, though, because they do nothing on their own. The only consequence is when you enable a plugin rule or use a plugin environment. I just don't see why, even in the demo context, you'd need to remove a plugin. As long as you just don't use it, it makes no difference.

I'm pretty skeptical that testing out plugins in the demo is going to work, so anything that is specifically for that use case I'd hold off on until you can put together a design for how its intended to work. As these would be public-facing, I don't want to add things because they seem like they're needed now and then be stuck with unnecessary or confusing methods.

I can see value to a general getRules() that returns all rules, regardless of the demo.

@ilyavolodin
Copy link
Member Author

Sounds good. We can start small and add as we need. Do you think it makes sense to have separate getRules and getRulesMetadata, or make it into a single call that returns everything?

@nzakas
Copy link
Member

nzakas commented Jun 27, 2016

I think if we stick with getRules() to start, we'll quickly be able to see if we need anything else.

Also, had another thought (and reinforces my belief that we should really discuss a design for the new, more powerful demo have you mind): I don't see any way that CLIEngine will behave in a browser environment. It seems like we might actually need a WebEngine to go along with it and that that object might be where we should be looking to add functionality for the demo.

@ilyavolodin
Copy link
Member Author

@nzakas While my only use-case is online demo, I don't think those APIs should be exclusive to demo only. For example, if somebody wanted to create rule configurator with a nice UI, they would need to get all schemas and metadata for rules.
That's also where I see ability to load/unload plugins come in handy as well.

@nzakas
Copy link
Member

nzakas commented Jun 27, 2016

@ilyavolodin I don't like inventing use cases that we haven't encountered before. Trying to solve a problem that we're not in the middle of will lead to bad decisions. Let's focus on the use case you actually have because it'll be easier to evaluate solutions for it. If and when someone else has a use case that requires other stuff, we can evaluate at that point in time.

@ilyavolodin
Copy link
Member Author

ilyavolodin commented Jun 27, 2016

Sounds good. In that case, primary use-case is online demo. Ability to retrieve list of all rules and schema/metadata for them would allow us to create more intelligent configuration selection interface. Let's skip plugins for now, since I don't know how something like that would work (although it's possible, since tonicdev already have an ability to load npm packages on the fly and execute them in the browser).

P.S. I updated original request with only necessary APIs.

@platinumazure
Copy link
Member

I look forward to seeing some of these new APIs implemented. If we do, then we can have eslint:recommended be a dynamic JS file that iterates through rules and returns those with meta.recommended = true, so we would only need to update one location (per rule) to add them to eslint:recommended.

Is there anything blocking consideration of adding getRules() and getRuleMetadata() at this point?

@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

If we do, then we can have eslint:recommended be a dynamic JS file that iterates through rules and returns those with meta.recommended = true, so we would only need to update one location (per rule) to add them to eslint:recommended.

There's nothing preventing us from doing that right now.

I think we only need version and getRules, since you can get metadata directly off of the rules.

@ilyavolodin
Copy link
Member Author

@nzakas To get metadata directly from the rules, you need to be able to access all of the rules. That's not a huge problem from inside ESLint, but if you are using ESLint as dependency, that will be an issue. Also even from inside ESLint, getting metadata for plugin rules would not be easy. I think we also need getRuleMetadata or we should return metadata as part of getRules.

@nzakas
Copy link
Member

nzakas commented Aug 10, 2016

@ilyavolodin sorry, I'm not sure what you're saying the problem is. Why can't I do this?

var meta = engine.getRules().semi.meta;

I'm unclear on why plugin rules would be different from any other case. Wouldn't that just be this?

var meta = engine.getRules()["plugin/rule"].meta;

If not, can you please be more descriptive and include some examples? I'm having trouble following this as an abstract idea.

@ilyavolodin
Copy link
Member Author

@nzakas I'm talking about getRules API as it is describe in my initial post, as in, it doesn't return actual rules.

Return should be an array of object with rule name, rule schema and current configuration.

Do you want to change that and return a full rule from that API call? I'm fine with that, and in that case we don't need getRuleMetadata, but then I have a slight concern about the size of returned array. Shouldn't be a problem in most cases, but if the target use-case is just getting information about the rule (schema, name, metadata, etc.) then returning everything is a bit of an overkill I think.

@nzakas
Copy link
Member

nzakas commented Aug 10, 2016

Ah, I was focusing on the first sentence in your description, "Should return all currently loaded rules." To me, that means you return whatever is required from the rule file. In general, it seems strange that getRules would return something other than the actual rule from the file. (Also, I have no idea what "current configuration" means in this context. There's no configuration until a file is a linted, right?)

To me, getRules should really just return a Map of rule ID to rule implementation (which is how we're representing it in rules.js currently). That way, all we are doing is returning a data structure we already have and don't need to do any additional work. From that Map, you should be able to do whatever you need to do (indeed, this is the only way that we typically work with rules, as well).

I'm not seeing a good argument for returning a subset of the rule information -- the size of that data really isn't a concern because we've already loaded it anyway. It seems like the only real difference from what you're proposing is the inclusion of all metadata instead of a subset and the creator function (which is already in memory).

@ilyavolodin
Copy link
Member Author

Sounds good to me. So let's have getRules return a map of rule names to implementation. In that case, we don't need getRulesMetadata at all.

@platinumazure
Copy link
Member

platinumazure commented Aug 10, 2016

My only concern is normalizing between old and new style rules (irrelevant in the case of core rules, but relevant for plugin rules which may be old style). Could/should the API normalize that at all? (It wouldn't be too hard to normalize an old style rule to new style, for example.)

Motivation: It would really stink for consumers to have to write if (typeof rule === "object") all the time. As they say, be tolerant in your input but strict in your output.

@nzakas
Copy link
Member

nzakas commented Aug 25, 2016

I think we already normalize somewhere.

@platinumazure
Copy link
Member

@nzakas Now that you say that, I think you might be right-- I've seen it in RuleTester. What we need to make sure of is that the normalization happens before the rule is defined into the rules config object.

@ilyavolodin ilyavolodin added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Sep 29, 2016
@ilyavolodin
Copy link
Member Author

TSC Summary:
This is the proposal to add two new API methods:

  • version in both linter and CLIEngine objects. Would return current ESLint version
  • getRules in linter object. Would return an array of all rules currently loaded.

@kaicataldo kaicataldo removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Sep 29, 2016
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 29, 2016
@kaicataldo
Copy link
Member

Marking as accepted as per the discussion during today's TSC meeting.

@sarbbottam
Copy link

sarbbottam commented Oct 8, 2016

getRules in linter object. Would return an array of all rules currently loaded.

getRules will return currently loaded w.r.t. to current eslint config and not all the available eslint rules.

Is my understanding correct?

@randycoulman
Copy link
Contributor

@sarbbottam My understanding from the discussion above is that all of the core ESLint rules will always be loaded and returned by getRules. But plugin rules will only be included if the plugin has been loaded first.

@ilyavolodin
Copy link
Member Author

@randycoulman That's correct. Core rules should be always loaded. Plugins rules will only return if the plugin is loaded.

@sarbbottam
Copy link

Thanks @randycoulman, @ilyavolodin.
With a little guidance I might be able to raise a PR.

I am not familiar with the code base, any pointer where should I start with?

@ilyavolodin
Copy link
Member Author

@sarbbottam I haven't looked around the code to see where would be the exact place to start with this, but in general this should go into linter object, which is an export from lib/eslint.js. So that would be a good place to start looking.

@mysticatea
Copy link
Member

Can we close this issue by #7669?

@ilyavolodin
Copy link
Member Author

Hmm... Why didn't it get closed automatically? Yes, this is now resolved. Closing.

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 enhancement This change enhances an existing feature of ESLint
Projects
No open projects
Development

No branches or pull requests

7 participants