-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add frontend linting rules for ESLint and Prettier #290
Conversation
42222cd
to
d722021
Compare
@Griffin-Sullivan can you please resolve the conflicts? |
d722021
to
b46e0f8
Compare
Ok rebased and linted some new stuff. CI should work now. @lucferbux could you take a look |
@@ -21,24 +21,24 @@ export const useSettings = (): { | |||
let cancelled = false; | |||
const watchConfig = () => { | |||
Promise.all([fetchConfig(), fetchUser()]) | |||
.then(([config, user]) => { | |||
.then(([fetchedConfig, fetchedUser]) => { |
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.
@lucferbux I had to change the name of config
and user
here because the linter didn't like that we were reusing the same name for the state variables.
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.
well, that's why we are using linters, great catch hahaha
@@ -25,33 +26,34 @@ export const useAdminSettings = (): NavDataItem[] => { | |||
// get auth access for example set admin as true | |||
const isAdmin = true; //this should be a call to getting auth / role access | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
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.
We will remove this when auth is implemented
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.
Can you add a todo in the code to add that info?
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.
Looks good, just one question there.
} | ||
], | ||
"no-restricted-properties": [ | ||
"error", |
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.
We don't actually have the allSettledPromises utility in this codebase yet (I think) - is this still something we want to enforce?
@lucferbux thoughts?
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.
We should probably remove it didn't notice that
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.
oh, this is a great catch, I think we don't use any concurrent call in Model Registry that uses allSettledPromises, I would remove it and add it in case we need it later.
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.
A couple of nits, but overall everything looks great.
} | ||
], | ||
"no-restricted-properties": [ | ||
"error", |
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.
oh, this is a great catch, I think we don't use any concurrent call in Model Registry that uses allSettledPromises, I would remove it and add it in case we need it later.
"optionalDependencies": true | ||
} | ||
], | ||
"no-relative-import-paths/no-relative-import-paths": [ |
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 was about to add something about disabling imports from other places that are not barrel files, but we have started a discussion right now, we might wanna wait and go on without those rules right now.
@@ -25,33 +26,34 @@ export const useAdminSettings = (): NavDataItem[] => { | |||
// get auth access for example set admin as true | |||
const isAdmin = true; //this should be a call to getting auth / role access | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
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.
Can you add a todo in the code to add that info?
<Route path="/admin" element={<Admin />} /> | ||
{ | ||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
isAdmin && <Route path="/admin" element={<Admin />} /> |
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.
And here too please?
@@ -21,24 +21,24 @@ export const useSettings = (): { | |||
let cancelled = false; | |||
const watchConfig = () => { | |||
Promise.all([fetchConfig(), fetchUser()]) | |||
.then(([config, user]) => { | |||
.then(([fetchedConfig, fetchedUser]) => { |
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.
well, that's why we are using linters, great catch hahaha
0e1fb82
to
c82b5c9
Compare
Verified comments resolved, tested locally. /approve |
/approve |
/lgtm (based on Lucas comments). |
/lgtm |
/approve |
/lgtm |
@tarilabs, would you mind merging this one for us if that's okay from your review? As it touches .gitHub I don't have enough permissions |
Bumps [arduino/setup-protoc](https://github.com/arduino/setup-protoc) from 2 to 3. - [Release notes](https://github.com/arduino/setup-protoc/releases) - [Commits](arduino/setup-protoc@v2...v3) --- updated-dependencies: - dependency-name: arduino/setup-protoc dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Griffin-Sullivan, would you mind looking at the conflicts? |
Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
c82b5c9
to
b22ff1f
Compare
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.
/lgtm
and #290 (comment)
/approve
thank you
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexcreasy, ederign, tarilabs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
Description
Adding the ESlint rules for the frontend and some Prettier options. I also updated our GitHub action to include the lint before running mock tests. This commit also lints everything that's why there's so many changes.
How Has This Been Tested?
npm run start:dev
npm run test:fix
npm run test:lint
npm run test:cypress-ci
npm run test
attempts to run both the lint and cypress-ci commandsMerge criteria: