mgr/dashboard: Migrate from tslint to eslint #45767
Conversation
Signed-off-by: Adithya Krishna <aadithya794@gmail.com>
Signed-off-by: Adithya Krishna <aadithya794@gmail.com>
|
jenkins test dashboard |
| // TODO: Change to error - From L58 to L67 | ||
| "@typescript-eslint/no-empty-function": "error", | ||
| "@typescript-eslint/member-ordering": "warn", | ||
| "no-underscore-dangle": "warn", | ||
| "@typescript-eslint/ban-types": "warn", | ||
| "prefer-arrow/prefer-arrow-functions": "warn", | ||
| "@typescript-eslint/consistent-type-assertions": "warn", | ||
| "@typescript-eslint/no-unused-expressions": "warn", | ||
| "max-len": "warn", | ||
| "@typescript-eslint/prefer-for-of": "warn", | ||
| "prefer-const": "warn", |
There was a problem hiding this comment.
@epuertat @nizamial09 As these eslint rules involved a lot of changes, I thought to take it in a separate PR once this is migration is done so that we can focus on not breaking anything when doing them. Hence I have added a TODO comment and made the rules to warn us
There was a problem hiding this comment.
Thanks a lot @adithyaakrishna for your work here! Plz let me know what you think about my suggestions!
| // TODO: Change to error - From L58 to L67 | ||
| "@typescript-eslint/no-empty-function": "error", | ||
| "@typescript-eslint/member-ordering": "warn", | ||
| "no-underscore-dangle": "warn", | ||
| "@typescript-eslint/ban-types": "warn", | ||
| "prefer-arrow/prefer-arrow-functions": "warn", | ||
| "@typescript-eslint/consistent-type-assertions": "warn", | ||
| "@typescript-eslint/no-unused-expressions": "warn", | ||
| "max-len": "warn", | ||
| "@typescript-eslint/prefer-for-of": "warn", | ||
| "prefer-const": "warn", |
| "**/node_modules/**" | ||
| "lintFilePatterns": [ | ||
| "src/**/*.ts", | ||
| "src/**/*.html" |
There was a problem hiding this comment.
Are JSON files linted by default?
| @@ -72,7 +72,7 @@ export class StartCaseBreadcrumbsResolver extends BreadcrumbsResolver { | |||
| resolve(route: ActivatedRouteSnapshot) { | |||
| const path = route.params.name; | |||
| const text = _.startCase(path); | |||
| return [{ text: `${text}/Edit`, path: path }]; | |||
| return [{ text: `${text}/Edit`, path }]; | |||
There was a problem hiding this comment.
We already discussed this in the original PR... While this is perfectly valid ES6 syntactic sugar, I think it's an ugly one, that may lead to future bugs as it creates tight coupling between internal variable names and external interfaces (e.g: if someone refactors this code and renames path to pathname, the resulting object will have a new interface). I'd vote for disabling this rule.
| import { SharedModule } from '~/app/shared/shared.module'; | ||
| import { FeatureTogglesGuardService } from '~/app/shared/services/feature-toggles-guard.service'; | ||
| import { ActionLabels, URLVerbs } from '~/app/shared/constants/app.constants'; |
There was a problem hiding this comment.
Any clue why these are moved to the bottom? Usually absolute imports come before than relative imports, and in this case they're reversed... 🤔 I'd suggest tuning this rule to keep absolute import firsts.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
| onNodeSelected(tree: TreeModel, node: TreeNode) { | ||
| TREE_ACTIONS.ACTIVATE(tree, node, true); | ||
| if (node.data.cdId) { | ||
| this.title = node.data.name; | ||
| const tempData = this.metadata[node.data.cdId] || {}; | ||
|
|
There was a problem hiding this comment.
Why this? Some method name reordering? It'd be great to know which rule is enforcing this...
The main risk with a linter or a static checker is that it may produce lots of changes that don't necessarily bring value (increased readability, code quality, etc), but cause a lot of code shuffling. And code shuffling will be a nightmare the next time we find a bug, we fix it in master and we realize that backporting that to a previous branch will require a lot of manual conflict solving because codebases diverged.
My suggestion would be to replace tslint with eslint disabling these cosmetic rules and ensuring that the amount of changes introduced is minimal (e.g.: 400 files vs. just 10).
From the eslint rules, I see there are: "possible problems", "suggestions" and "layout". Probably we can start focusing on the 1st category, and checking what changes that outputs. Additionally, I'd compare the other categories with the equivalent ones defined by tslint, and ensure equivalent rules are enabled. If you needed it you can create a doc/etherpad/etc., where you list the tslint vs eslint rules, and we can help you there if needed.
| let deepFlatten: any; | ||
| let layering: any; | ||
| let exclusiveLock: any; | ||
| let objectMap: any; | ||
| let journaling: any; | ||
| let fastDiff: any; |
There was a problem hiding this comment.
since each variable is defined separately, shouldn't they be aligned?
|
jenkins test dashboard cephadm |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
Signed-off-by: Adithya Krishna aadithya794@gmail.com
Fixes: https://tracker.ceph.com/issues/48258
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows