-
Notifications
You must be signed in to change notification settings - Fork 889
Conversation
src/language/rule/rule.ts
Outdated
} | ||
|
||
export interface IRule { | ||
getOptions(): IOptions; | ||
isEnabled(): boolean; | ||
apply(sourceFile: ts.SourceFile): RuleFailure[]; | ||
applyWithWalker(walker: IWalker): RuleFailure[]; |
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 method was never necessary to have here.
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.
should also not be removed for now
src/ruleLoader.ts
Outdated
@@ -43,23 +38,19 @@ export function loadRules(ruleOptionsList: IOptions[], | |||
|
|||
for (const ruleOptions of ruleOptionsList) { | |||
const ruleName = ruleOptions.ruleName; | |||
const enableDisableRules = enableDisableRuleMap.get(ruleName); | |||
if (ruleOptions.ruleSeverity !== "off" || enableDisableRuleMap) { |
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.
It's not necessary to check ruleSeverity
here since isEnabled()
will do that too.
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.
technically you are right, but this check was added to do less work for disabled rules. With your change you need to find the rule in rulesDirectory and instantiate it before you know if you will ever need it (for every linted file).
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.
Isn't the real problem that we re-load every rule for every file? (Cached, but still.) The only SourceFile-specific things here are ruleDisabler
and isJs
. Why not eagerly load the rules up-front (including disabled ones and TypeScript-only ones)?
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.
If you use tslint as a library, you can pass different rule configs for every call to Linter.lint
.
That's why this needs to be done for every file.
On the other hand, I don't know if that makes sense to keep that "feature".
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.
OK, added the check back.
@@ -78,9 +78,6 @@ var AAAaA = 'test' // previously both `quotemark` and `variable-name` rules were | |||
var AAAaA = 'test' // tslint:disable-line:quotemark | |||
var AAAaA = 'test' // ensure that disable-line rule correctly handles previous `enable rule - disable all` switches | |||
|
|||
// tslint:enable:no-var-keyword |
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 conflicts with an assertion in ruleLoaderTests.ts
that it "loads disabled rules if rule in enableDisableRuleMap". (It also now conflicts with enable-via-comment
.) Which should we choose?
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.
The behaviour to not load disabled rules was added recently. Even though the rule was loaded, it would have never been executed, because rule.isEnabled()
always returned false. I don't know why the test was implemented this way...
I think it makes more sense to keep disabled rules disabled. Otherwise you need to guess the ruleSeverity
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.
What if someone wants a rule disabled by default but enabled in a particular file?
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.
I though about that too. That's a valid use case. But the problem with the missing severity still stands.
If you could enable/disable a rule and set its severity independently, that wouldn't be a problem.
Or just use a default severity (#2279)
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.
I have a test case enable-via-comment
where a rule is turned off, but is enabled in the file and gives errors there. This is accomplished by using rule.isEnabled() || ruleDisabler.isExplicitlyEnabled(ruleName)
instead of just isEnabled()
. What other test cases would need to work?
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.
On that topic: All of the custom implementations of isEnabled()
I see are just turning it off in the case of bad options. We should instead throw an error on bad options. I think just like Rules are no longer responsible for tracking their disabled ranges, they shouldn't be responsible for tracking whether they're enabled or not. (As shown here, it especially doesn't make sense if we want to know whether a rule is enabled before loading it.) So we should remove isEnabled()
from the Rule
interface. Would that help solve this problem?
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.
What I'm actually trying to say: You disable a rule by setting severity: off
. Now you turn it on via comment. What severity should this rule have? How do I set a specific severity other than the default?
To summarize both inline discussions: |
src/rules/maxFileLineCountRule.ts
Outdated
return ruleFailures; | ||
|
||
return this.applyWithFunction(sourceFile, (ctx) => | ||
ctx.addFailure(0, 1, Rule.FAILURE_STRING(lineCount, lineLimit))); |
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.
you could directly return the failure now that applyWithFunction
doesn't modify the result anymore
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.
I like this better than constructing new Lint.RuleFailure
directly because I don't have to pass in ruleName
explicitly. Could have a Rule#createFailure
method but I think that would only be used here.
EDIT: nvm, it's actually shorter with new Lint.RuleFailure
.
src/rules/maxFileLineCountRule.ts
Outdated
const lineCount: number = sourceFile.getLineStarts().length; | ||
const disabledIntervals = this.getOptions().disabledIntervals; | ||
|
||
if (lineCount > lineLimit && disabledIntervals.length === 0) { |
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 changes the behavior of the rule, which should at least be documented.
Before this change the rule would not fail if there were any disabledInvervals. Now you would need to have a disable comment in the first line and first column to disable the rule.
You would only get back the old behavior if the failure would span the whole file. I would not suggest doing that.
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.
Moved error to the bottom, so a tslint:disable
anywhere should work. Presumably no one uses tslint:disable-line
for this rule.
Related, but no blocker for this PR: when applying fixes, the position of the disabledIntervals may no longer be correct. It needs to be refreshed after each rule that applied fixes. |
In order to keep this PR small, I've removed |
src/rules/eoflineRule.ts
Outdated
new Lint.RuleFailure(sourceFile, length, length, Rule.FAILURE_STRING, | ||
this.getOptions().ruleName), | ||
]); | ||
return this.applyWithFunction(sourceFile, (ctx) => |
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.
here too
it doesn't look like any unit tests hit enableDisableRules.ts |
There is a black-box test |
Test failure fixed by #2691. |
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.
I don't know if this is intended to be a breaking change. If it is, just ignore most of my comments below
src/language/rule/abstractRule.ts
Outdated
return this.filterFailures(ctx.failures); | ||
} | ||
|
||
protected filterFailures(failures: RuleFailure[]): RuleFailure[] { |
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.
breaking change. keep this method, just return the input unchanged and deprecate it
src/language/rule/rule.ts
Outdated
@@ -18,7 +18,11 @@ | |||
import * as ts from "typescript"; | |||
|
|||
import {arrayify, flatMap} from "../../utils"; | |||
import {IWalker} from "../walker"; | |||
|
|||
export interface RuleStatic { |
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.
that's a duplicate of RuleConstructor
?
@@ -99,19 +103,12 @@ export interface IOptions { | |||
ruleArguments: any[]; | |||
ruleSeverity: RuleSeverity; | |||
ruleName: string; | |||
disabledIntervals: IDisabledInterval[]; |
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 should be kept and deprecated for backwards compatibility
src/language/rule/rule.ts
Outdated
disabledIntervals: IDisabledInterval[]; | ||
} | ||
|
||
export interface IDisabledInterval { |
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.
same as above
src/language/rule/rule.ts
Outdated
} | ||
|
||
export interface IRule { | ||
getOptions(): IOptions; | ||
isEnabled(): boolean; | ||
apply(sourceFile: ts.SourceFile): RuleFailure[]; | ||
applyWithWalker(walker: IWalker): RuleFailure[]; |
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.
should also not be removed for now
export function getSourceFile(fileName: string, source: string): ts.SourceFile { | ||
const normalizedName = path.normalize(fileName).replace(/\\/g, "/"); | ||
return ts.createSourceFile(normalizedName, source, ts.ScriptTarget.ES5, /*setParentNodes*/ true); | ||
} | ||
|
||
export function doesIntersect(failure: RuleFailure, disabledIntervals: IDisabledInterval[]) { |
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.
another breaking change
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.
I don't think this is public. (#2446)
@nchen This was originally written during tslint 4.0. What is the schedule for new major tslint releases? It seemed to be much slower in the past but the gap from 4.0 to 5.0 was pretty small. If it will be more than a few months before the next major-version release I can add dummy implementations over the breaking changes as @ajaff mentioned. Otherwise I would be fine with waiting a little longer. |
There isn't a major release scheduled. It's usually based on how many breaking changes are queued up |
f78dc11
to
9cdbb18
Compare
Everything in language/utils.ts is exported. For example as Lint.hasModifier
Am 06.05.2017 18:56 schrieb "Andy Hanson" <notifications@github.com>:
… ***@***.**** commented on this pull request.
------------------------------
In src/language/utils.ts
<#2369 (comment)>:
> export function getSourceFile(fileName: string, source: string): ts.SourceFile {
const normalizedName = path.normalize(fileName).replace(/\\/g, "/");
return ts.createSourceFile(normalizedName, source, ts.ScriptTarget.ES5, /*setParentNodes*/ true);
}
-export function doesIntersect(failure: RuleFailure, disabledIntervals: IDisabledInterval[]) {
I don't think this is public. (#2446
<#2446>)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2369 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ALaeKLE5PTwR9hNCcIoi_6q16RF9_Mbtks5r3KYjgaJpZM4Mhg9M>
.
|
src/linter.ts
Outdated
// tslint:disable-next-line member-ordering | ||
protected applyFixes(sourceFilePath: string, source: string, fixableFailures: RuleFailure[]): string { | ||
if (fixableFailures.some((f) => !f.hasFix())) { | ||
throw new Error("!"); |
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.
needs error text
let source: string = sourceFile.text; | ||
|
||
for (const rule of enabledRules) { | ||
const hasFixes = fileFailures.some((f) => f.hasFix() && f.getRuleName() === rule.getOptions().ruleName); |
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.
not that we should optimize on the --fix code path, but wouldn't it be more efficient to do:
- call rule1
- fix rule1
- call rule2
- fix rule2
instead of
- call all rules
- call rule1 again if there were fixes
- fix rule1
- call rule2 again if there were fixes
- fix rule2
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.
Well, the way the enable/disable logic is done here, it's better to handle disables for all rules at once. Otherwise we would have to parse disables after every rule.
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.
you could reuse the disable map and reload it when the file is overwritten. I'm ok with how it is now, though
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.
I would hold off on that until #1330 is implemented, since that will need to know all the uses of the disable map.
PR checklist
Overview of change:
disabledIntervals
out of rule options. Rules are not responsible for disabling theirselves. Just filter the failures after the rule returns them.filterFailures
entirely, removing code that looked for duplicate failures. It turns out a few rules were returning duplicate failures, so I changed them directly.disabledIntervals
at all. Just have aRuleDisabler
interface and a method for getting one from the disable comments in a source file.IDisabledInterval
.Is there anything you'd like reviewers to focus on?
There were conflicting tests as to whether explicitly enabling a rule turned off in tslint.json would enable it. I chose that it would. If you don't want that, the
isExplicitlyEnabled
method could be removed.CHANGELOG.md entry:
[no-log] change are internal and backwards compatible