Improve frontend lint settings #969

DaoDaoNoCode · 2023-02-21T20:34:20Z

Description

closes frontend part of #469

Frontend:

Use ~/... instead of relative path
Use return as needed (get rid of the return statement when there is only the return in the function body)
Require to use curly for all the statements (if, while, etc...)
Encourage to name variables in camelCase
Prevent else return and absolute import from the src folder like services/**, pages/**, etc...
Require to read all the methods in the ~/api folder not to use the specific file path
Add no-console rule
Add order rules for imports

How Has This Been Tested?

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
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

…ixed

…lute path. add no-param-reassign

andrewballantyne

@DaoDaoNoCode is there a reason we are not excluding from 'directory/file' now that we have ~? Eg frontend/src/services/notebookService.ts has import { RecursivePartial } from 'typeHelpers'; and you passed the linter.

andrewballantyne · 2023-02-23T19:17:41Z

frontend/src/utilities/allSettledPromises.ts

-export const allSettledPromises = <T, E extends unknown = undefined>(
+export const allSettledPromises = <T, E = undefined>(


Interesting... not sure why I did that originally.

DaoDaoNoCode · 2023-02-23T19:50:12Z

@andrewballantyne I thought of that, and at first, I tried to create a glob pattern matching like */**, but it will filter the imported from packages like react-dom/client. There are actually two ways, and I don't know which one is better.

block all the path like */** and create an allowlist for the paths like ~/**, ./** and node packages we are using
create a blocklist for the paths like services/**, types, pages/**, and don't block any other paths

Which option do you think is better?

andrewballantyne · 2023-02-23T22:34:19Z

@andrewballantyne I thought of that, and at first, I tried to create a glob pattern matching like */**, but it will filter the imported from packages like react-dom/client. There are actually two ways, and I don't know which one is better.

block all the path like */** and create an allowlist for the paths like ~/**, ./** and node packages we are using

create a blocklist for the paths like services/**, types, pages/**, and don't block any other paths

Which option do you think is better?

Hmmmm -- are you sure there is no way to just block non npm dependencies? Surely there is a webpack setting or something; I swear I recall this being a thing 🤔 I'd be shocked if there was no way to stop root-level imports like from 'directory' because it should say "that's not an npm import", just thinking about it.

Gkrumbach07 · 2023-02-27T22:24:30Z

Is there a reason to use the tilde ~/.... We can get the same effect of accessing root level folders by just omitting ~/

Example - these have the same outcome assuming folder structure has folder at the root level.

import { ... } from "folder"

import { ... } from "~/folder"

andrewballantyne · 2023-02-27T22:32:07Z

Is there a reason to use the tilde ~/.... We can get the same effect of accessing root level folders by just omitting ~/

Example - these have the same outcome assuming folder structure has folder at the root level.
import { ... } from "folder"
import { ... } from "~/folder"

Yuppers -- because import { ... } from 'thing' is an npm import. Overloading that has no value. Putting ~ prefix indicates that it is not an npm package.

Will we conflict with a folder and a npm package? Probably not... but it is the same as "why use ; at the end of every line". There are things we do for ease of use, common acceptance for new comers, and in general just a style we choose. I don't see any value in compounding concepts for ease of use.

Is there a reason you're against it @Gkrumbach07 ?

Gkrumbach07 · 2023-02-27T22:44:22Z

Is there a reason you're against it @Gkrumbach07 ?

Nope, it just seemed like extra chars at first glance. But the reasons to use it make sense

andrewballantyne

Approving and merging to avoid the expected conflicts that will happen when ever single PR merges before this -- easier for them to adjust than for you to adjust.

One note I found with no-console... definitely can be a follow up though.

andrewballantyne · 2023-02-28T02:45:21Z

frontend/src/app/useTimeBasedRefresh.ts

@@ -39,6 +40,7 @@ const useTimeBasedRefresh = (): SetTime => {
        `We should have refreshed but it appears the last time we auto-refreshed was less than an hour ago. '${KEY_NAME}' session storage setting can be cleared for this to refresh again within the hour from the last refresh.`,
      );
    }
+    /* eslint-enable no-console */


Hmmm... wonder if we can disable block disable/enables -- seems easy to abuse.

openshift-ci · 2023-02-28T02:49:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* update config of eslint, webpack, ts, jest and dependencies * run lint fix command to get rid of most linting errors * manually get rid of all the warnings and errors that cannot be auto fixed * manually add ~ alias to all previous absolute imports in frontend * add rule to prevent else return and absolute import from src folder * another round of lint --fix * move mock files and change imports * add no-console rule and disable console as needed in code * add restriction to import from api and change the rules of using absolute path. add no-param-reassign * add order rule and run lint fix command * remove baseUrl from tsconfig and make corresponding changes to eslintrc --------- Co-authored-by: Andrew Ballantyne <8126518+andrewballantyne@users.noreply.github.com>

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Feb 21, 2023

openshift-ci bot requested review from Gkrumbach07 and lucferbux February 21, 2023 20:34

DaoDaoNoCode added 4 commits February 22, 2023 10:32

update config of eslint, webpack, ts, jest and dependencies

2f6bba6

run lint fix command to get rid of most linting errors

702052b

manually get rid of all the warnings and errors that cannot be auto f…

35e4587

…ixed

manually add ~ alias to all previous absolute imports in frontend

dd2cb43

DaoDaoNoCode force-pushed the upstream-issue-469 branch from 8b9d74b to dd2cb43 Compare February 22, 2023 16:04

DaoDaoNoCode added 6 commits February 22, 2023 14:09

add rule to prevent else return and absolute import from src folder

17063bc

another round of lint --fix

608830c

move mock files and change imports

b9fa27d

add no-console rule and disable console as needed in code

ad72aa0

add restriction to import from api and change the rules of using abso…

5c62fcb

…lute path. add no-param-reassign

add order rule and run lint fix command

18e0be2

DaoDaoNoCode changed the title ~~[WIP] Improve lint settings~~ Improve frontend lint settings Feb 23, 2023

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Feb 23, 2023

andrewballantyne reviewed Feb 23, 2023

View reviewed changes

remove baseUrl from tsconfig and make corresponding changes to eslintrc

f902b68

openshift-merge-robot added the needs-rebase PR needs to be rebased label Feb 27, 2023

Merge branch 'main' into upstream-issue-469

f61630a

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Feb 27, 2023

andrewballantyne approved these changes Feb 28, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Feb 28, 2023

openshift-ci bot added the lgtm label Feb 28, 2023

openshift-ci bot added the approved label Feb 28, 2023

openshift-merge-robot merged commit 1d27a18 into opendatahub-io:main Feb 28, 2023

andrewballantyne linked an issue Feb 28, 2023 that may be closed by this pull request

[Conversation]: Improve Linter Settings #469

Closed

andrewballantyne mentioned this pull request Feb 28, 2023

[Conversation]: Improve Linter Settings #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve frontend lint settings #969

Improve frontend lint settings #969

DaoDaoNoCode commented Feb 21, 2023 •

edited

Loading

andrewballantyne left a comment

andrewballantyne Feb 23, 2023

DaoDaoNoCode commented Feb 23, 2023

andrewballantyne commented Feb 23, 2023

Gkrumbach07 commented Feb 27, 2023 •

edited

Loading

andrewballantyne commented Feb 27, 2023

Gkrumbach07 commented Feb 27, 2023

andrewballantyne left a comment

andrewballantyne Feb 28, 2023

openshift-ci bot commented Feb 28, 2023

		export const allSettledPromises = <T, E extends unknown = undefined>(
		export const allSettledPromises = <T, E = undefined>(

Improve frontend lint settings #969

Improve frontend lint settings #969

Conversation

DaoDaoNoCode commented Feb 21, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Feb 23, 2023

Choose a reason for hiding this comment

DaoDaoNoCode commented Feb 23, 2023

andrewballantyne commented Feb 23, 2023

Gkrumbach07 commented Feb 27, 2023 • edited Loading

andrewballantyne commented Feb 27, 2023

Gkrumbach07 commented Feb 27, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Feb 28, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Feb 28, 2023

DaoDaoNoCode commented Feb 21, 2023 •

edited

Loading

Gkrumbach07 commented Feb 27, 2023 •

edited

Loading