Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

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

Open
donaldpipowitch opened this issue Apr 25, 2017 · 13 comments
Open

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

donaldpipowitch opened this issue Apr 25, 2017 · 13 comments

Comments

@donaldpipowitch
Copy link

donaldpipowitch commented Apr 25, 2017

Given palantir/tslint#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.
@blakeembrey
Copy link
Collaborator

@donaldpipowitch Why not just use the extends feature as recommended in your PR (palantir/tslint#2163 (comment))?

@donaldpipowitch
Copy link
Author

donaldpipowitch commented Apr 25, 2017

Because of my comment below 😄 palantir/tslint#2163 (comment)

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
Copy link
Contributor

ajafff commented Apr 25, 2017

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

@blakeembrey
Copy link
Collaborator

blakeembrey commented Apr 25, 2017

@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
Copy link
Author

donaldpipowitch commented Apr 25, 2017

@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 palantir/tslint#2358 (comment). 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
Copy link
Contributor

ajafff commented Apr 25, 2017

@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
Copy link
Author

@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
Copy link
Owner

buzinas commented Apr 25, 2017 via email

@blakeembrey
Copy link
Collaborator

@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
Copy link
Author

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
Copy link

taylon commented Nov 28, 2017

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.

ikokostya added a commit to yandex/mapsapi-codestyle that referenced this issue Jan 14, 2018
@mt-micky
Copy link

mt-micky commented May 2, 2018

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
Copy link
Contributor

boycce commented Aug 25, 2018

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 added a commit to boycce/tslint-eslint-rules that referenced this issue Aug 25, 2018
jmlopez-rod pushed a commit that referenced this issue Aug 25, 2018
[docs] improved README.md file (discussed in #214)
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

7 participants