-
Notifications
You must be signed in to change notification settings - Fork 530
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
FIx eslint warnings #1792
FIx eslint warnings #1792
Conversation
…fix linting errors Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
…hu-kun/jaegertracing#1723 Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
'@typescript-eslint/no-explicit-any': 0, | ||
'@typescript-eslint/no-unused-vars': 0, | ||
'@typescript-eslint/no-var-requires': 0, | ||
'@typescript-eslint/no-empty-function': 0, | ||
'@typescript-eslint/ban-types': 0, | ||
'@typescript-eslint/ban-ts-comment': 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.
I think this change defeats the purpose of fixing the eslint warnings. We don't have to turn them off, instead, have to fix 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.
Majority of warning are caused by any
or empty objects and functions. And I don't know why they use any
in codebase I think it's not accidental, it may be solves some purpose and those empty object or function represents default state of something or they used in types.
There are three ways to doing that:
- One is I can turn off them instance level.
- Another one is I can ignore them at file level.
- And last one is I can ignore them at project level.
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.
Some or most of the time, any
acts as an escape from typescript's strict typing. That doesn't mean that it can't be replaced with a more narrower typedef.
Anyway, that is about '@typescript-eslint/no-explicit-any': 0,
, what about the other ones?
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.
working on!
Hey, I think you should have another branch for the changes specific to fixing eslint warnings. Otherwise reviewing the files won't be possible IMO. |
I don't know what happening but I was created a different branch all together. |
Maybe you selected the branch's origin as your icon-related branch instead of jaeger-ui/main |
Yeah I was selected the origin |
@@ -627,7 +627,7 @@ exports[`<DdgNodeContent> renders correctly when decorationValue is a string 2`] | |||
<span | |||
className="DdgNodeContent--actionsItemIconWrapper" | |||
> | |||
<MdVisibilityOff /> | |||
<IoEyeOff /> |
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 are the icon changes in this PR that is meant for fixing lint warnings?
Which problem is this PR solving?
Resolves #1608
Description of the changes
yarn lint
. This improves code quality, consistency, and reduces technical debt.Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test