-
Notifications
You must be signed in to change notification settings - Fork 397
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
Update eslint rules and refactoring types. #2676
Update eslint rules and refactoring types. #2676
Conversation
miettal
commented
Jun 22, 2024
- banned using Object, Function types
- refactor .eslintrc
1. eslint native rules 2. unused-imports 3. typescript-eslint 4. angular-eslint 5. stylistic
"@stylistic/quote-props": [ | ||
"warn", | ||
"consistent" | ||
], |
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.
could you fix formatting, why did you change the order around?
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.
to clear rule cateogry. I commented on this commit.
4a1365c
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 agree
@@ -126,7 +126,7 @@ export class Service extends AbstractService { | |||
// this.notify(notification); | |||
} | |||
|
|||
public setCurrentComponent(currentPageTitle: string | { languageKey: string, interpolateParams?: Object }, activatedRoute: ActivatedRoute): Promise<Edge> { | |||
public setCurrentComponent(currentPageTitle: string | { languageKey: string, interpolateParams?: {} }, activatedRoute: ActivatedRoute): Promise<Edge> { |
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 better than Object
but still not good, anyway this method is deprecated and shouldnt be used in the future anyway
"{}": false, | ||
"Object": false, | ||
"Function": false | ||
"{}": 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 would ban empty object as well, otherwise, type safety is not guaranteed
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.
Yes I want to ban {}. but in this repo there are using many {} type. especially {} used in json rpc related type. this means if I remove {} in this repo, this will be super heavy task(Many typing will needed....). so I focused to ban Object and Function.
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.
Reducing any
type and banning {}
, applying strictNullChecks is very very important...
if I have time, I will address this kind little by little in other PR.
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 understand in terms of being a heavy task, so i approve
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.
thx for your contribution 🚀