-
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
refactor: migrate TSLint rules to ESLint equivalents #4158
Conversation
revert another Record changePreviews: 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.
self-review
@@ -16,7 +16,7 @@ | |||
|
|||
if (typeof require !== "undefined" && typeof window !== "undefined" && typeof document !== "undefined") { | |||
// we're in browser | |||
// tslint:disable-next-line:no-var-requires | |||
// eslint-disable-line @typescript-eslint/no-var-requires |
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.
invalid comment flag, should be removed
.eslintrc.js
Outdated
// HACKHACK: test dependencies are only declared at root but used in all packages. | ||
"import/no-extraneous-dependencies": "off", | ||
// HACKHACK: many violations, should be fixed eventually | ||
"import/no-internal-modules": "off" |
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 can be removed since I ended up re-enabling tslint no-submodule-imports
@@ -181,7 +182,7 @@ describe("CompareUtils", () => { | |||
/* Empty */ | |||
} | |||
} | |||
// tslint:enable:max-classes-per-file | |||
// eslint-enable max-classes-per-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.
this probably needs to be a block comment /* */
const red = (rgb >> 16) & 0xff; | ||
const green = (rgb >> 8) & 0xff; | ||
const blue = (rgb >> 0) & 0xff; | ||
// tslint:enable | ||
/* eslint-enable */ |
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: specify which rule is being enabled
@@ -93,7 +95,7 @@ interface IPanelExampleState { | |||
counter: number; | |||
} | |||
|
|||
// tslint:disable-next-line:max-classes-per-file | |||
// eslint-disable-line max-classes-per-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.
extraneous comment
@@ -23,7 +23,7 @@ interface IBigSpaceRock { | |||
[key: string]: number | string; | |||
} | |||
|
|||
// tslint:disable-next-line:no-var-requires | |||
// eslint-disable-line @typescript-eslint/no-var-requires |
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.
is this actually working? it should be "eslint-disable-next-line"...
@@ -92,7 +92,7 @@ install typings for Blueprint's dependencies before you can consume it: | |||
npm install --save @types/react @types/react-dom | |||
``` | |||
|
|||
Blueprint's declaration files require **TypeScript 2.3+** for default generic parameter arguments: `<P = {}>`. | |||
Blueprint's declaration files require **TypeScript 2.3+** for default generic parameter arguments: `<P = Record<string, unknown>>`. |
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 revert this
packages/eslint-config/tslint.json
Outdated
"file-header": { | ||
"options": [ | ||
"Copyright \\d{4} Palantir Technologies, Inc\\. All rights reserved.", | ||
"Copyright 2019 Palantir Technologies, Inc. All rights reserved.\n\nLicensed under the Apache License, Version 2.0 (the \"License\");\nyou may not use this file except in compliance with the License.\nYou may obtain a copy of the License at\n\n http://www.apache.org/licenses/LICENSE-2.0\n\nUnless required by applicable law or agreed to in writing, software\ndistributed under the License is distributed on an \"AS IS\" BASIS,\nWITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\nSee the License for the specific language governing permissions and\nlimitations under the License." |
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 be "2020"
packages/landing-app/src/index.scss
Outdated
@@ -129,8 +129,10 @@ footer { | |||
background-image: linear-gradient( | |||
90deg, | |||
rgba($white, 0) 0%, | |||
rgba($white, 0.3) 50%, | |||
rgba($white, 0) 100% | |||
rgba($white, 0.3) |
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.
revert unrelated change
@@ -80,7 +80,7 @@ function walk(ctx: Lint.WalkContext<void>) { | |||
function getAllMatches(className: string) { | |||
const ptMatches = []; | |||
let currentMatch: RegExpMatchArray | null; | |||
// tslint:disable-next-line:no-conditional-assignment | |||
// eslint-disable-line no-cond-assign |
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.
disable-next-line ?
fixes for self-review commentsPreviews: documentation | landing | table |
This PR migrates lint rule configuration from TSLint rules to the closest possible ESLint equivalents.
tslint-to-eslint-config
initially produced a few warnings:In addition, some of the conversions were semantically different, so I reverted to the TSLint version. The remaining TSLint rule config can be seen in
packages/tslint-config/tslint.json
. Note that@blueprintjs/tslint-config
was not changed, and it doesn't really need to change for our use cases right now, but at some point we will deprecate it and ultimately remove its code from this repository.Some general issues so far:
@typescript-eslint/ban-types
banning{}
by default is problematic, the suggestion ofRecord<string, unknown>
is not very usefulRemaning migration TO DO items (outside this PR):