Skip to content
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

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

Griffin-Sullivan
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan commented Aug 21, 2024

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?

  1. Run the frontend with npm run start:dev
  2. Run the linting fix command with npm run test:fix
  3. Run the linter with npm run test:lint
  4. Run the test suite with npm run test:cypress-ci
  5. Make sure npm run test attempts to run both the lint and cypress-ci commands

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@ederign
Copy link
Member

ederign commented Aug 26, 2024

@Griffin-Sullivan can you please resolve the conflicts?

@Griffin-Sullivan
Copy link
Contributor Author

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]) => {
Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

@alexcreasy alexcreasy left a 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",
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

@lucferbux lucferbux left a 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",
Copy link
Contributor

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": [
Copy link
Contributor

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
Copy link
Contributor

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 />} />
Copy link
Contributor

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]) => {
Copy link
Contributor

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

@Griffin-Sullivan Griffin-Sullivan force-pushed the ui-lint branch 2 times, most recently from 0e1fb82 to c82b5c9 Compare August 28, 2024 12:45
@alexcreasy
Copy link
Contributor

Verified comments resolved, tested locally.

/approve

@alexcreasy
Copy link
Contributor

/approve

@alexcreasy
Copy link
Contributor

/lgtm (based on Lucas comments).

@alexcreasy
Copy link
Contributor

/lgtm

@alexcreasy
Copy link
Contributor

/approve

@ederign
Copy link
Member

ederign commented Aug 28, 2024

/lgtm

@ederign
Copy link
Member

ederign commented Aug 28, 2024

@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

dhirajsb pushed a commit to dhirajsb/model-registry-kfp that referenced this pull request Aug 30, 2024
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>
@ederign
Copy link
Member

ederign commented Aug 30, 2024

@Griffin-Sullivan, would you mind looking at the conflicts?

Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Aug 30, 2024
Copy link
Member

@tarilabs tarilabs left a 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

Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit e5e6f73 into kubeflow:main Aug 30, 2024
14 checks passed
Al-Pragliola pushed a commit to Al-Pragliola/model-registry that referenced this pull request Sep 3, 2024
Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants