-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create a fixer for classes-constants lint rule #2969
Conversation
1. The lint rule will now fail the entire node, instead of just the pt-* string 2. The lint rule now has a fixer that will replace the pt-* string with the blueprint import
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.
woooo thanks @alexthemark!
@@ -19,7 +18,7 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
optionsDescription: "Not configurable", | |||
optionExamples: ["true"], | |||
type: "style", | |||
typescriptOnly: false, | |||
typescriptOnly: false |
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 think this'll fail the lint check -- make sure you're respecting the repo's tslint.json
(it's at the root)
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.
Sorry about that. My IDE had prettier running by default.. and I didn't notice that I prettified your files for no additional fee! Will fix
function shouldIgnoreBlueprintClass(blueprintClassName: string): boolean { | ||
if (blueprintClassName.startsWith("icon")) { | ||
// Icon conversions happen elsewhere. | ||
return true; |
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.
return blueprintClassName.startsWith("icon")
let stringWithoutPtClasses = node.getText(); | ||
ptClassStrings.forEach(cssClass => { | ||
stringWithoutPtClasses = stringWithoutPtClasses.replace(cssClass, ""); | ||
}); |
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 would work nicely as a reduce
. then let stringWithoutPtClasses
could be a const
.
), | ||
); | ||
} else { | ||
const replacement = `\`${templateStrings}\``; |
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 could be combined with line 97 since the replacements.push()
is repeated.
const replacement = ptClassStrings.length === 1 ? convertPtClassName(ptClassStrings[0]) : `\`${templateStrings}\``
|
||
// taken from tslint orderedImportRules | ||
function compare(a: string, b: string): 0 | 1 | -1 { | ||
function isLow(value: string) { |
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.
pull this out to root scope to avoid redefining it each time compare()
is called.
"module": "commonjs", | ||
"outDir": "../lib/rules" | ||
"outDir": "../lib/rules", | ||
"skipLibCheck": true |
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.
why?
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.
Previously, I didn't include the dom library, which led to some of the @types types in the root node_modules directory being angry. I've now added the dom library, even though it's unused, since I think that seems like the lesser of two evils.
|
||
<Button className="my-class pt-large" /> | ||
~~~~~~~~ [msg] | ||
~~~~~~~~~~~~~~~~~~~ [msg] |
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.
😞 why does this mark the entire string now instead of just the offending portion like it used to? feels like a regression.
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.
Unfortunately, the tslint replacement api only lets me provide a replacement for a failed node. Previously, blueprint could fail the string by character positions, but now I have to fail the entire node so that I can replace it.
Agreed this is worse for seeing the lint errors... but it is better because it gets rid of them!
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 i see that.
is it possible to report the failure in one location and the replacement on a wider location? so we could have both worlds.
@alexthemark test flake in react 16. rerun from failed please. |
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.
nice!!
Fixes #0000
Checklist
Changes proposed in this pull request:
Create a fixer for classes-constants lint rule
This does not create a fixer for icons, since that rule is slightly more complex, and should handle the options for creating an
<Icon {...} />
element, or usingIconNames
Reviewers should focus on:
Lint rule semantics/test cases.