Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Rule documentation questions #590

Closed
jfmengels opened this issue May 26, 2016 · 19 comments
Closed

Rule documentation questions #590

jfmengels opened this issue May 26, 2016 · 19 comments

Comments

@jfmengels
Copy link
Contributor

jfmengels commented May 26, 2016

Hi there!

First off, thanks for building this tool, it's really great!

I just discovered the rule documentation feature (this one), and thought it was pretty neat. there's even my name in there, so 馃槃

I have a few questions and remarks:

  1. Do you want to support any eslint plugin you come accross, even if the list gets very long? What about deprecated packages? They probably should stay, but if you decide they shouldn't, then mine (eslint-plugin-import-order), should be removed.
  2. Pretty sure you thought about it already, and decided not to, but wanted to confirm it: Have you thought about always displaying a link to https://github.com/<repo>/eslint-plugin-<name>/blob/master/docs/rules/${ruleName}.md? Most plugins follow that convention I think (1 counter-example in the list currently).
    Maybe the linter could attempt to make a HTTP request to see if the page exists, and link to the the contribution page if it doesn't. There could still be an exception list.
  3. Shouldn't this list be in another repository? That way this could be used by other linters in Atom (xo for instance) or on other editors. It could get published more often because it wouldn't need a release of this linter to be updated.
  4. This doesn't support scoped packages. There aren't a lot, but I know we have a public scoped one at my workplace. If somehow point 2 gets implemented, it might be nice (willing to help out on this).

And again, thanks for making this great tool! :)

@Arcanemagus
Copy link
Member

  1. If somebody cared enough about a plugin to add it to the list, then it's still being used. As long as the repo still exists I don't see any harm in having it listed?
  2. Personally I'd rather just stick to the current method where users are directed to the Wiki page on adding a new plugin to the list (that looks like it needs to be finished still) than possibly sending them to a 404 with no idea where to go from there. Although it's probably possible to do a request to check, it would majorly slow down reporting results to the user since it could only be checked after the results were returned from eslint.
  3. Since the list is specific to eslint plugins, I'm not sure how it would be useful to any other linter provider...? The one you linked is a rather unique example in that it looks to just be an automatically configuring eslint 馃槙.
  4. Do scoped packages report their rules differently within eslint internally? This uses the ruleID, not the package name that provides the rule.

@Arcanemagus
Copy link
Member

Marking as closed since there was no further response.

@jfmengels
Copy link
Contributor Author

jfmengels commented Jun 10, 2016

Sorry about the lack of response, my bad...

  1. If somebody cared enough about a plugin to add it to the list, then it's still being used. As long as the repo still exists I don't see any harm in having it listed?

Don't see a problem with it either, mostly curious about this. I deprecated my plugin, and it is being downloaded more than ever, so yeah, probably better to just keep supporting them.

2 Personally I'd rather just stick to the current method where users are directed to the Wiki page

Very understandable choice. The experience would be so much better though if it was possible while not degrading the user experience. I think it's pretty hard to do pre-fetching as you don't go and fetch the list of used ESLint rules yourself, right?

3 Since the list is specific to eslint plugins, I'm not sure how it would be useful to any other linter provider...? The one you linked is a rather unique example in that it looks to just be an automatically configuring eslint

In the case of atom-linter-xo, it's a XO wrapper (in the sense that linter-eslint is a layer over ESLint), and yeah, XO does pre-configure ESLint, but it does add a few things currently lacking in ESLint (but mostly it's really pre-configuring).

I think this could also be used for other editor linters (probably not Sublime as it's written in Python), but the linter for Visual Studio Code, or simply eslint formatters (they might give you a clickable link in the console for instance).

By the way, if this should be moved to another repo, the repo could be set up in a simpler way (no compiling I mean), and people could contribute using only the GitHub website (kind of like this).

4 Do scoped packages report their rules differently within eslint internally? This uses the ruleID, not the package name that provides the rule.

I'd have to check, as I haven't written/seen any yet (so maybe you should wait until there's a real live example and request for it), but the problem with what I've seen is the way the ruleId is parsed

  const ruleParts = ruleId.split('/')

  if (ruleParts.length === 1) {
    return `http://eslint.org/docs/rules/${ruleId}`
  }

  const pluginName = ruleParts[0]
  const ruleName = ruleParts[1]

I'm pretty sure that the rule would be configured with @scope/my-plugin/rule-name, so you'd get pluginName = "@scope", plugin = "my-plugin" and rule-name would be left out.

@Arcanemagus
Copy link
Member

I think it's pretty hard to do pre-fetching as you don't go and fetch the list of used ESLint rules yourself, right?

Correct, this plugin just requests eslint to tell it what is wrong with the text content of a file, and reports back the returned rules in the Linter API format for consumption by whatever called it. Delaying the response to check each and every message's generated link would be a bit ridiculous 馃槢.

I think this could also be used for other editor linters

I was talking about other Atom providers, but I can see how a generic list like that could work for others. I personally am not interested in maintaining that separately, though if somebody wanted to set that up and submitted a PR here utilizing it I'd be fine with moving over to it 馃槈.

I'm pretty sure that the rule would be configured with @scope/my-plugin/rule-name, so you'd get pluginName = "@scope", plugin = "my-plugin" and rule-name would be left out.

That's how scoped packages look when they are specified in the eslint configuration, but I'm not sure how they would look in the rule ID returned in the message object. @IanVS do you know?

@IanVS
Copy link
Member

IanVS commented Jun 10, 2016

Honestly I'm not sure, no. I've not used scoped packages before. Should be fairly easy to figure out with some experimenting, but I don't have time at the moment.

@Arcanemagus
Copy link
Member

Arcanemagus commented Jun 10, 2016

Me either, we can figure it out when somebody actually uses them and (potentially) runs into an issue I guess 馃槢.

@jfmengels
Copy link
Contributor Author

jfmengels commented Jun 20, 2016

if somebody wanted to set that up and submitted a PR here utilizing it I'd be fine with moving over to it 馃槈.

I'm interested in doing this. @sindresorhus would you be interested in having this functionality in XO / atom-linter-xo? (I'm referring to point 3 in the original comment, if you want to re-read the whole thread ^^')

@sindresorhus
Copy link

@jfmengels 馃憤

@jfmengels
Copy link
Contributor Author

jfmengels commented Jul 31, 2016

FYI, finally got to working on this, and I finally created that separate repo https://github.com/jfmengels/eslint-rule-documentation/. If you have some feedback before I publish this, I'm all ears! Thanks!

@IanVS
Copy link
Member

IanVS commented Aug 1, 2016

Sweet, I like it. Would you be willing to submit a PR to linter-eslint to switch us over to using it?

@jfmengels
Copy link
Contributor Author

jfmengels commented Aug 1, 2016

Would you be willing to submit a PR to linter-eslint to switch us over to using it?

Yes. I'll try to find some time for this, and I'll see how it goes (I have never worked with atom plugins before, and I would like to test it properly when working on it ;))

Btw, I have added @Arcanemagus and @IanVS as collaborators for the repo. Feel free to add the people you deem apt to maintain, I don't know who worked on this the most.

@jfmengels
Copy link
Contributor Author

FYI: An already nice use of this list in eslint-find-rules

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

@jfmengels FYI It looks like ESLint may be adding support for the rules themselves to specify a path to their own documentation: eslint/eslint#6582

I think your package will still be useful as a fallback for packages which do not specify doc urls though. (assuming this change even happens.)

@jfmengels
Copy link
Contributor Author

Ha, good to know, thanks for the info.
That would complicate the tool quite a bit more, but yeah, it would still be useful as a fallback, especially as quite a few plugins don't respect the "standard" ESLint plugin structure.

I'll try to follow the story, but let me know if I missed something in the development of this :)

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

Yeah I don't think it actually impacts your repo too much, I think it would be up to linter-eslint to fall back to yours iif the rule message we get from ESLint does not have a url.

@jfmengels
Copy link
Contributor Author

Ah, just now figured out that the url would appear in the problem object. Then yeah, I wouldn't have to update my repo and it would be just used as a fallback. That'd be great yeah!

I actually thought this was proposing some kind of convention or something to have the doc field in the meta, so that we could fetch it by requiring the rule. Would be a lot more complicated.

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

It's still in proposal phase so far, but yes, it looks like the current proposal would add the meta to the problem object.

@jfmengels
Copy link
Contributor Author

Yeah. I like it. That would allow a lot more flexibility and nice things to appear.
An example I have in mind is to show a different color for the error based on whether it's a possible error or a stylistic issue.
This is available in the core rules' meta category field, and they could become a standard for other plugins.

@pmcelhaney
Copy link

eslint/eslint#6582 has finally been implemented.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants