search

buzinas / tslint-eslint-rules

Improve your TSLint with the missing ESLint rules
MIT License
712 stars 105 forks source link

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

Open donaldpipowitch opened 7 years ago

donaldpipowitch commented 7 years ago

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:

blakeembrey commented 7 years ago

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

donaldpipowitch commented 7 years ago

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.

ajafff commented 7 years ago

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

blakeembrey commented 7 years ago

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

donaldpipowitch commented 7 years ago

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

ajafff commented 7 years ago

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

donaldpipowitch commented 7 years ago

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

buzinas commented 7 years ago

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.

blakeembrey commented 7 years ago

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

donaldpipowitch commented 7 years ago

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.

taylon commented 6 years ago

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.

mt-micky commented 6 years ago

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

boycce commented 6 years ago

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.