front: fix simple oxlint violations - #18456
Conversation
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Note that we need to await on `act` even though its signature looks like it's not awaitable; it's because TS do not understand that it *can* (and should) be awaited depending on React's version. Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
bf40d22 to
3de0043
Compare
Synar
left a comment
There was a problem hiding this comment.
Thanks for fixing all these issues!
| @@ -48,7 +48,7 @@ function getConflictTrainNames( | |||
| // Otherwise, the name is `${pacedTrainName}/+` | |||
| const namedException = trainSchedule.paced.exceptions.find( | |||
| // TODO_EXCEPTION: remove `!` when using TrainSchedulingException type | |||
There was a problem hiding this comment.
I believe this TODO can now be dropped
|
|
||
| const withControlledValue: Decorator = (Story, ctx) => { | ||
| const [value, setValue] = useState<string>(String(ctx.args.value ?? '')); | ||
| const [value, setValue] = useState<string>(String((ctx.args.value as string) ?? '')); |
| <span className="mr-3"> | ||
| {((schema.properties || {})[k] as JSONSchema7 | undefined)?.title || k} | ||
| </span> | ||
| {/* oxlint-disable-next-line typescript/no-base-to-string */} |
There was a problem hiding this comment.
I believe you can get rid of this disable by changing the unkwown in LinearMetadataTooltip signature to string | number (which should be the only values allowed by LinearMetadataItem, so no loss of generality).
There was a problem hiding this comment.
Woah, good catch indeed! Thanks for that, I wouldn't have thought about that 👍
| </div> | ||
| <div>at: {hovered.point.at}</div> | ||
| {/* eslint-disable-next-line typescript/no-base-to-string */} | ||
| <div>caused by: {hovered.point.source?.toString()}</div> |
There was a problem hiding this comment.
Shouldn't this be a JSON.stringify instead?
There was a problem hiding this comment.
I didn't dug because it was in DebugMap, but indeed, a JSON-stringified version is good enough 👍
| if (value === '') return null; | ||
|
|
||
| /* eslint-disable-next-line typescript/no-base-to-string */ | ||
| const stringValue = `${value}`; |
There was a problem hiding this comment.
Perhaps this should also be a JSON.stringify instead
| "prefer-nullish-coalescing": "off", | ||
| "prefer-readonly-parameter-types": "off", | ||
| "prefer-tag-over-role": "off", | ||
| "radix": "off", |
There was a problem hiding this comment.
I'm unconvinced this rule is useful to us, though I guess trimming this list is nice
| @@ -48,7 +48,7 @@ function getConflictTrainNames( | |||
| // Otherwise, the name is `${pacedTrainName}/+` | |||
| const namedException = trainSchedule.paced.exceptions.find( | |||
| // TODO_EXCEPTION: remove `!` when using TrainSchedulingException type | |||
There was a problem hiding this comment.
We should remove the TODO as well, I think.
|
|
||
| const withControlledValue: Decorator = (Story, ctx) => { | ||
| const [value, setValue] = useState<string>(String(ctx.args.value ?? '')); | ||
| const [value, setValue] = useState<string>(String((ctx.args.value as string) ?? '')); |
There was a problem hiding this comment.
Hm, unfortunately it doesn't seem like React.InputHTMLAttributes.value field is specialized based on the input type… I would argue that we should Omit the value field from TextAreaProps and set it explicitly as a string to avoid irrelevant types such as number and string[]. That sounds all orthogonal to this PR though.
If we cast to string here, then we don't need to pass the result to String(): it's already a string. ?? '' is also unnecessary according to the expression's type. Either the cast should be as (string | undefined), or the cast should be on the result of value ?? ''.
There was a problem hiding this comment.
Fully agree. I don't think we could have undefined here in runtime, so let's simplify it as useState<string>(ctx.args.value as string) which makes sense in such a Storybook's story.
| Top = 0, | ||
| Bottom = 1, | ||
| Left = 2, | ||
| Right = 3, |
There was a problem hiding this comment.
Ah, these originate from NGE. We should no longer need this file, see #18458.
| )) | ||
| ); | ||
|
|
||
| /* eslint-disable-next-line typescript/await-thenable */ |
There was a problem hiding this comment.
Since we're using a locked and recent React version, I don't think we'll ever run into the Promise version of this?
| @@ -48,7 +48,7 @@ function getConflictTrainNames( | |||
| // Otherwise, the name is `${pacedTrainName}/+` | |||
| const namedException = trainSchedule.paced.exceptions.find( | |||
| // TODO_EXCEPTION: remove `!` when using TrainSchedulingException type | |||
We have a very long list of rules that we disable in
.oxlintrc.json. Those settings were grandfathered from some old eslint configuration, but it's quite beneficial to reduce this list, as those lints can find actual code smells!In this pull request, I fix 15 ones that I deemed both uncontroversial and easy to fix. A commit-by-commit review is highly recommanded: each commit contains the name of the lint being re-enabled.
I started from the rules that had the least number of violations in our codebase, but there are still 55 rules left after this pull request, some more controversial, but most of them still uncontroversial but with way more violations.