-
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
add new lint rules to ease migration #2394
Conversation
fix lint errors in our codebase due to classes-constantsPreview: documentation | landing | table |
move stuff inside rules/ dirs, compile to lib/rules/Preview: documentation | landing | table |
@@ -0,0 +1,42 @@ | |||
/* |
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.
hmm, this package is only intended for @blueprintjs
packages, right? So it doesn't make sense to put rules that will be consumed by end-user apps here, does it?
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.
🤔 after this PR, it will contain some blueprint-specific rules, but this package is intended for consumption by who-so-ever wishes.
i proposed putting these rules here or in core
itself and we agreed that this package makes more sense as it actually depends on tslint
. i'll migrate the internal tslint-config to depend on this (where most of the config came from in the first place).
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.
https://github.com/palantir/tslint-blueprint was designed for rules like this, consider putting these there
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.
Seems a little late for that! Looks like that package already has this rule. But makes sense for these things to live in the blueprint repo, no?
Also 👋👋 hi @adidahiya!!
move rules to new tslint-rules packagePreview: documentation | landing | table |
test linting failuresPreview: documentation | landing | table |
compile rules before linting in CIPreview: documentation | landing | table |
another lint testPreview: documentation | landing | table |
@@ -16,7 +14,7 @@ const fakeFocusEngine = { | |||
|
|||
const focusEngine = | |||
typeof document !== "undefined" | |||
? new InteractionModeEngine(document.documentElement, FOCUS_DISABLED_CLASS) | |||
? new InteractionModeEngine(document.documentElement, "pt-focus-disabled") |
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.
expecting lint failure
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.
success ✅
*/ | ||
|
||
/** | ||
* Enable Blueprint-specific TSLint rules defined in this package. |
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.
move code back to tslint-config, add blueprint-rules.js modulePreview: documentation | landing | table |
re-fix lintingPreview: documentation | landing | table |
packages/tslint-config/README.md
Outdated
{ | ||
"extends": [ | ||
"@blueprintjs/tslint-config", | ||
"@blueprintjs/tslint-config/blueprint-rules" |
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 add an example with only this line and make it the basic usage example. There will be a lot more people I think who will want to just use the BP rules than use all the rules.
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'll def add a note that the rules can be used on their own
packages/tslint-config/index.js
Outdated
@@ -25,11 +25,6 @@ module.exports = { | |||
// ["assert", "equal", "use assert.strictEqual instead"] |
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 file should extend blueprint-rules.js
, right?
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.
no i thought that was the whole point of this... that the base config does not include any blueprint-specific stuff?
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.
nah, the purpose is the inverse of that actually, that you can use the blueprint-specific rules (such as no icon strings) without pulling in the rest of the config
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 so default config includes blueprint, but you can also just use blueprint without config?
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! because most apps using BP are going to want to pick up the BP rules and config that say "don't use the .pt- prefix" but not pick up the general TS lint rule config
Check out the README for usage instructions. |
}; | ||
// tslint:enable:object-literal-sort-keys | ||
|
||
public static COMPONENT_MESSAGE = "use <NameIcon /> component for icon prop"; |
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.
sort of makes me think <NameIcon />
is an actual component
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 might be able to improve the messaging here so it actually mentions icon name, but we gotta merge #2356 first. and i def want to add a fixer for this.
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 now it's NamedIcon
add README and LICENSE to tslint-config packagePreview: documentation | landing | table |
🤖 remove default file-header config for non-palantir usersPreview: documentation | landing | table |
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.
nits
.circleci/config.yml
Outdated
@@ -51,6 +51,7 @@ jobs: | |||
keys: | |||
- v7-dependency-cache-{{ checksum "yarn.lock" }} | |||
- v7-dependency-cache- | |||
- run: yarn compile --scope "@blueprintjs/tslint*" |
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.
since circle runs yarn compile
above with no scope, shouldn't this already be done?
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.
lint
and compile
run in parallel so they don't affect each other 😢
@@ -26,7 +26,10 @@ describe("Spinner", () => { | |||
it("value sets stroke-dashoffset", () => { | |||
// dash offset = X * (1 - value) | |||
const root = mount(<Spinner value={0.35} />); | |||
assert.isTrue(root.find(`.${Classes.SPINNER}`).hasClass(Classes.SPINNER_NO_SPIN), "missing class pt-no-spin"); | |||
assert.isTrue( | |||
root.find(`.${Classes.SPINNER}`).hasClass(Classes.SPINNER_NO_SPIN), |
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.
will this also fail if the class can't be found? (i'm trying to infer why assert.isTrue
was used...
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.
"assert that the spinner is not spinning"
@@ -336,11 +336,11 @@ describe("<DateInput>", () => { | |||
wrapper.setState({ isOpen: true }); | |||
|
|||
// try typing a new time | |||
wrapper.find(".pt-timepicker-millisecond").simulate("change", { target: { value: "1" } }); | |||
wrapper.find("." + Classes.TIMEPICKER_MILLISECOND).simulate("change", { target: { value: "1" } }); |
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.
nit: we use backtick interpolation elsewhere. might as well be consistent.
@@ -52,7 +52,7 @@ export class GuideLayer extends React.Component<IGuideLayerProps, {}> { | |||
left: `${offset}px`, | |||
}; | |||
const className = classNames(Classes.TABLE_OVERLAY, Classes.TABLE_VERTICAL_GUIDE, { | |||
"pt-table-vertical-guide-flush-left": offset === 0, | |||
[`${Classes.TABLE_VERTICAL_GUIDE}-flush-left`]: offset === 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.
nit: since our classes have configurable PREFIX, then this will technically work, but it's worth noting this is slightly different that the other uses since we're building a classname string.
packages/tslint-config/README.md
Outdated
const element = <div className={Classes.NAVBAR} />; | ||
``` | ||
|
||
```json |
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.
Change json
to js
to disable highlighting error? Vanilla JSON doesn't allow comments.
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.
json5
will also work
major readme editsPreview: documentation | landing | table |
add fixers to icon-components rule!Preview: documentation | landing | table |
revert tslint config change, lintPreview: documentation | landing | table |
Fixes #2393
@blueprintjs/tslint-config
package:blueprint-classes-constants
enforces use ofClasses.*
constants instead of"pt-prefixed-strings"
.blueprint-icon-components
enforcesicon="tick"
oricon={<TickIcon />}
usage (requires [icons] Enable tree-shaking for SVG icons #2356 for icon components). this rule is disabled in the default config, as it's an opt-in decision to ensure tree shaking of icons is possible.file-header
rule config as it references Palantir (still exists in the repo config).Check out the README for usage instructions.