-
Notifications
You must be signed in to change notification settings - Fork 161
Upgrade eslint and migrate config #4456
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
Conversation
6f336e3
to
4f10c0a
Compare
Regarding the check errors. All of the For the try {
...
} catch { There is no need for the error check at this point as it's simply used to set an empty inventory That would leave 2 errors
Dealing with these two in the order shown then for const defaultInitialState = () => ({
name: "",
namespace: "",
source: "",
path: "",
});
type FormState = ReturnType<typeof defaultInitialState>; may be better written as simply type FormState = {
name: string;
namespace: string;
source: string;
path: string;
}; I'm still not sure how to proceed with the I would be inclined to add a per-line disable for this check with a // TODO: Investigate if this can be removed safely via a component upgrade path
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
d3; |
Thanks for the solid review with all the explanations/suggestions for a non-frontend developer, @mproffitt! I will address your suggestions tomorrow! |
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.
Tests currently don't run locally for me but they do run in CI and are successful which is the main thing 🟢
UI still starts and executes in Tilt and a quick visual inspection of the UI shows no apparent changes or breakages have been introduced through fixing lint errors in the code.
Overall for a first F/E PR this is fairly solid work.
@casibbald or @tenstad, are one you able to approve this PR, so we can get it merged? Or add @mproffitt as an eligible approver? I can probably do the latter, but it feels wrong for me to adjust such settings as a newcomer to this project. |
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.
Fingers crossed
Closes #4355 #4418
What changed?
Why was this change made?
We should keep up with releases of tools, as the usually get better.
How was this change implemented?
How did you validate the change?
Release notes
Documentation Changes