tslint-eslint-rules icon indicating copy to clipboard operation
tslint-eslint-rules copied to clipboard

add empty "index.js" to "rules/" directory

Open donaldpipowitch opened this issue 8 years ago • 13 comments

Given https://github.com/palantir/tslint/issues/2163 it would be nice, if the rules/ directory would contain an empty index.js. Then we could resolve the rules directory with Nodes module resolution which makes it easy to use custom rules in certain situations (symlinked configs, flat vs. non-flat installations...).

(Update)

Motivation:

  • extends: Allows me to use custom rules from other packages. I get all rules with default configs and can opt-out from rules. If new rules are added in a new version I get them automatically.
  • rulesDirectory : Allows me to use custom rules from other packages. I opt-in to use certain rules with my specific configs. If new rules are added in a new version I don't get them automatically.

donaldpipowitch avatar Apr 25 '17 13:04 donaldpipowitch

@donaldpipowitch Why not just use the extends feature as recommended in your PR (https://github.com/palantir/tslint/issues/2163#issuecomment-293416322)?

blakeembrey avatar Apr 25 '17 15:04 blakeembrey

Because of my comment below 😄 https://github.com/palantir/tslint/issues/2163#issuecomment-293466893

AFAIK extends gives me all rules with all default values. I'd like to "opt-in" to rules instead of "opt-out" from rules. If new rules are added with new versions I don't know, if I want this rule.

donaldpipowitch avatar Apr 25 '17 19:04 donaldpipowitch

the config provided by tslint-eslint-rules contains only the rulesDirectory and no rules. It's made exactly for your use case

ajafff avatar Apr 25 '17 19:04 ajafff

@donaldpipowitch Fair enough. The original comment didn't make any sense when I read it. The main concern I have (apart from having to add empty files around the place to make people happy) is that this could have adverse affects tying the directory structure to the module. Is there maybe a better way this feature could be implemented in TSLint to avoid that? For instance, could it do regular resolution and then use the rulesDirectory property instead of you having to know where the rules live. Not against it in the end, but it seems unnecessary to create hacks like empty files.

Edit: Realised dir-resolve was the initial PR - looked at the wrong commit. Also, what if one package had two rulesDirectory settings, is it expected you'd copy it twice vs just extending "rules" from it once?

blakeembrey avatar Apr 25 '17 19:04 blakeembrey

@ajafff

the config provided by tslint-eslint-rules contains only the rulesDirectory and no rules. It's made exactly for your use case

Sorry, I'm confused. When I do extends: [ "tslint-eslint-rules" ] what configs do I get? Isn't it this file?

@blakeembrey

Not against it in the end, but it seems unnecessary to create hacks like empty files. Another note, shouldn't the directory already be resolving because you added dir-resolve as a dependency there?

The reviewers recommended to remove dir-resolve later. See here https://github.com/palantir/tslint/pull/2358#issuecomment-289023170. Sorry, I'll update the first post in the PR.

I'd prefer to use dir-resolve, too. So nobody would need to add empty files.

donaldpipowitch avatar Apr 25 '17 19:04 donaldpipowitch

@donaldpipowitch you get this file: https://github.com/buzinas/tslint-eslint-rules/blob/master/index.js because of this line: https://github.com/buzinas/tslint-eslint-rules/blob/master/package.json#L5

Instead of adding an empty index.js, it could be moved to dist/rules/index.js (or any other basename) and let main in package.json point to that file instead. With that setup you can either "extends": "tslint-eslint-rules" or "rulesDirectory": "tslint-eslint-rules"

ajafff avatar Apr 25 '17 20:04 ajafff

@ajafff Thank you for the clarification. So currently I could use extends: [ "tslint-eslint-rules" ] with my desired behaviour, but this could change in the future (if default settings are added) and it is specific to tslint-eslint-rules (because other packages like tslint-reactdo set default rules, right? here and here).

donaldpipowitch avatar Apr 25 '17 20:04 donaldpipowitch

The goal of this project is to make it possible to people who use TypeScript to easily migrate an ESLint configuration to TSLint. We're not opinionated about what rules you should use, and never will.

We only created the "extends" option because people thought it was simpler to use than rulesDirectory, but we will never add any configuration by default.

buzinas avatar Apr 25 '17 20:04 buzinas

@ajafff That might be a reasonable way to make use of both features in one go, nice! 😄

Would it make sense to make a request to TSLint to make it possible to extend with only rulesDirectory instead of inheriting config?

We'd need to document the directory also in the README, I don't think it's enough just to add an empty index.js - probably deserves a README section and a comment in the file for the next person to run into the file (whether we use the existing index.js file moved or not).

blakeembrey avatar Apr 25 '17 20:04 blakeembrey

Small feedback to the README.md: I actually thought that I'd get all rules wich are marked as (recommended) in the description with extends.

donaldpipowitch avatar Apr 26 '17 06:04 donaldpipowitch

I think that using "rulesDirectory" like suggested by @donaldpipowitch would be an amazing good practice for rule packages in general.

That way "rulesDirectory" would always be the way to "import" the rules and "extend" is the way to enable a preset, that is more like ESLint btw, where you have "plugins" for new rules and "extend" for presets (correct me if I'm wrong on this one).

While tslint-eslint-rules do not set any default rules, packages like tslint-react for example does. Making it super confusing for newcomers (like myself) to figure out what is going on.

taylon avatar Nov 28 '17 19:11 taylon

I'm currently writing a set of rules to share across my company and be able to use rulesDirectory would make much more sense. Here a work around: https://github.com/progre/tslint-config-airbnb/blob/master/tslint.js#L6

mt-micky avatar May 02 '18 04:05 mt-micky

You guys need to remove the use of extends since there are no default presets, its not correct and is confusing, especially when reading through the readme.

boycce avatar Aug 25 '18 01:08 boycce