feat(configuration): set js rules to all valid active rules #3641
Conversation
|
Thanks for your interest in palantir/tslint, @mlavina! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
src/configuration.ts
Outdated
| const ruleOptions = parseRuleOptions(config[ruleName], defaultSeverity); | ||
| if (copyRulestoJsRules && ruleOptions.ruleSeverity !== "off") { | ||
| const Rule = findRule(ruleName, rulesDirectory); | ||
| if (Rule === undefined || (Rule.metadata && Rule.metadata.typescriptOnly)) { |
There was a problem hiding this comment.
Is there a nicer way to find if a rule is typescript only? Because for this i need to pass in rulesDirectory which makes the parent parse function a bit less clean
mlavina
force-pushed
the
feat/configuration/setJsRulesToAllValidActiveRules
branch
3 times, most recently
from
6dfc904
to
b0a4364
Compare
|
CLA has been signed. |
docs/usage/configuration/index.md
Outdated
| @@ -29,7 +29,7 @@ A path to a directory or an array of paths to directories of [custom rules][2]. | |||
| - A boolean value may be specified instead of the above object, and is equivalent to setting no options with default severity. | |||
| - Any rules specified in this block will override those configured in any base configuration being extended. | |||
| - [Check out the full rules list here][3]. | |||
| * `jsRules?: any`: Same format as `rules`. These rules are applied to `.js` and `.jsx` files. | |||
| * `jsRules?: any | boolean`: Same format as `rules` or you can set to true to copy over all valid active rules from `rules`. These rules are applied to `.js` and `.jsx` files. | |||
There was a problem hiding this comment.
"Same format as rules or explicit true to copy all valid active rules from rules."
src/configuration.ts
Outdated
| function parseRules(config: RawRulesConfig | undefined): Map<string, Partial<IOptions>> { | ||
| function parseRules(config: RawRulesConfig | undefined, | ||
| copyRulestoJsRules = false, | ||
| rulesDirectory?: string | string[]): Map<string, Partial<IOptions>> { |
There was a problem hiding this comment.
please move copyRulesToJsRules feature to a separate function, rather than co-opting this existing method into doing multiple things.
mlavina
force-pushed
the
feat/configuration/setJsRulesToAllValidActiveRules
branch
from
b0a4364
to
45a6868
Compare
|
@giladgray Good call on separating the function. It is done tell me what you think? |
mlavina
force-pushed
the
feat/configuration/setJsRulesToAllValidActiveRules
branch
from
45a6868
to
3f6a3df
Compare
jsRules can now be a boolean. If it is set to true then parseConfig file, will copy over all active rules that can be applied to js to the jsRules configuration.
mlavina
force-pushed
the
feat/configuration/setJsRulesToAllValidActiveRules
branch
from
3f6a3df
to
306e0c8
Compare
|
i rebased to the latest branch hopefully, that solves the ci:next failing. @astorije @giladgray Are you okay with the requested changes i made? |
ghost
mentioned this pull request
|
@giladgray sorry to be annoying but just checking in if you had a chance to look at the changes I made per your request. And if there is anything else i can do to get this merged in. |
|
I'll be "that guy" and ping this again. Looks like there's a bunch of people (myself included) looking for this feature and it seems to be ready to go. @giladgray are you the one to sign off or is there someone else that should look at this? |
|
Any plans for an actual release? I've been waiting in anticipation for this feature for a while now! |
Let tslint check our JS (configuration) files in the project root, too. With the release of palantir/tslint#3641 (I subscribed) the manual [prototypical] re-specification of `jsRules` will hopefully be obsolte soon.
|
Looking forward to this next release too, but @mlavina, I might be wrong but assuming how the code is implemented, it will not take into account rules extended from |
|
when can we except new release with this functionality ? |
|
@kevinmarrec I think that depends on how extends works internally. If extends actual tracks the rules internally this should work with extends. I feel like that is something I would have tested, but to be honest it's been a while since I wrote this |
We can define jsRules as a boolean true, so all the rules defined in key "rules" that are compatible with js will be copied to here. If you want to know more about this feature: palantir/tslint#3641.
jsRules can now be a boolean. If it is set to true then parseConfig file, will copy over all active rules that can be applied to js to the jsRules configuration.
PR checklist
Overview of change:
I added comments to the files changed
Is there anything you'd like reviewers to focus on?
CHANGELOG.md entry: