-
Notifications
You must be signed in to change notification settings - Fork 885
inherit defaultSeverity onto child configurations #3240
Conversation
Thanks for your interest in palantir/tslint, @felschr! 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. |
I have no clue why the build failed. Can someone have a look at this?
UPDATE |
Sorry for all the commits regarding the style issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs a test, Have a look at test/configurationTests.ts
around line 280 for related tests
// assign defaultSeverity of parent if child configuration has no defaultSeverity set: | ||
const parentDefaultSeverity = parentConfig !== undefined ? parentConfig.defaultSeverity : undefined; | ||
rawConfigFile.defaultSeverity = | ||
rawConfigFile.defaultSeverity !== undefined ? rawConfigFile.defaultSeverity : parentDefaultSeverity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a defaultSeverity
specified in the extending config file should always override the defaultSeverity
in all extended configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's should still be the case with my additions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider the following:
- a.json has no defaultSeverity
- b.json has defaultSeverity "warning"
- c.json extends a.json and b.json and has defaultSeverity "error"
The result is:
- rules specified in c.json get a default of "error"
- rules from b.json get a default of "warning", because b.json specifies its own defaultSeverity
- rules from a.json get a default of "error" from c.json, because a.json has no defaultSeverity.
To override the defaultSeverity in b.json, you'd need to adjust this line to
rawConfigFile.defaultSeverity = parentDefaultSeverity !== undefined ? parentDefaultSeverity : rawConfigFile.defaultSeverity;
To make it clearer what's happening here, I suggest to rename parentConfig
to extendingConfig
@ajafff Thanks for directing me where to look. |
@felschr Any progress on the PR? Our team experience the same problem and would really like the fix to be merged soon. |
@M0ns1gn0r sorry, I wanted to do this but haven't really had time. |
Superseded by #3449 |
PR checklist
Overview of change:
As reported in #2569 setting the defaultSeverity in a configuration that extends another configuration (e.g.
tslint:recommended
) leaves the severity of the extended rules unchanged.The changes in this pull-request make the defaultSeverity inherited from the top-most configuration to all underlying ones (by
extends
&ruleDirectory
).Is there anything you'd like reviewers to focus on?
This is mainly a quick for me personally but I'm open to improvement.
E.g. this could be activated only by a flag (e.g.
--inheritDefaultSeverity
).Also some people in #2569 mentioned they'd like to completely override all defaults. This could also be added as a 2nd flag (e.g.
--forceDefaultSeverity
).Depending on the feedback I can adjust the code and I'll also create new unit tests if necessary.
CHANGELOG.md entry:
[enhancement]
inherit defaultSeverity onto child configurations