🌱 Setup eslint with prettier #1291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjd78
force-pushed
the
fix_eslint
branch
2 times, most recently
from
3ef6615
to
e791084
Compare
mturley
reviewed
Aug 24, 2023
| "@typescript-eslint/ban-types": "warn", | ||
| "@typescript-eslint/no-explicit-any": "warn", | ||
| "react/jsx-key": "warn", | ||
| "react-hooks/rules-of-hooks": "warn", |
There was a problem hiding this comment.
IMO the rules-of-hooks rule should be "error" since conditionally-called hooks can cause really strange broken behaviors.
There was a problem hiding this comment.
I agree, 100%.
But...
I want to get this PR to the state where we can merge it w/o also merging a huge set of auto-fixes and getting in the way of custom assessment development.
I plan on a followup PR for each one of these rules where:
- the "warn" setting gets removed (defaulting to error)
- all of the offending code is fixed
After all of the warnings are gone, the code base will be in a better place.
|
Now includes the commit from #1316 |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
=======================================
Coverage 43.04% 43.04%
=======================================
Files 145 145
Lines 4330 4330
Branches 999 999
=======================================
Hits 1864 1864
Misses 2454 2454
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
ibolton336
pushed a commit
that referenced
this pull request
Aug 28, 2023
While working on upgrading eslint (PR #1291), a switch/case `no-fallthrough` error was found in the `getIconByRisk` function in the `AnswerTable` component. This error would cause a risk of "red" to actually render as a risk of "yellow". Fixing it separately from the eslint upgrade seems like a good thing. Signed-off-by: Scott J Dickerson <[email protected]>
mturley
approved these changes
Aug 28, 2023
ibolton336
pushed a commit
to ibolton336/tackle2-ui
that referenced
this pull request
Aug 29, 2023
While working on upgrading eslint (PR konveyor#1291), a switch/case `no-fallthrough` error was found in the `getIconByRisk` function in the `AnswerTable` component. This error would cause a risk of "red" to actually render as a risk of "yellow". Fixing it separately from the eslint upgrade seems like a good thing. Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
fix_eslint
branch
2 times, most recently
from
a0372a9
to
f72a077
Compare
Replace the old and broken eslint configuration in `client/package.json` with a more robust configuration. `eslint`, `typescript-eslint` and `prettier` are configured to work together to lint files in the client package. Note: The current versions of configurations require `prettier@^3`, but the `pretty-quick` command only works with `prettier@^2`. The husky git pre-commit hook needs to be updated to another package to provide the same kind of functionality (try `lint-staged`). ---- Add configs for prettier, adjust eslint rules For prettier, there are some config changes between v2 and v3. The new default for tailing commas changed causing a lot of breaks. Use the v2 default of "es5" for now. A few eslint rules changed to warnings since there are a lot of them and we don't want to fix those now. ---- Enable linting for all monorepo packages ---- Resolve lint errors for common and server packages ---- Set more eslint rules to warn to avoid mass fixes ---- Fix the current set of eslint errors A small set of changes were required for 39 total errors across a small set of files. Rather than mark those errors to the warning level and fixed in another PR, the fixes have been made here. ---- Use `lint-staged` as pre-commit hook Previously `pretty-quick` was used to run `prettier@^2` in the client package when committing staged files. Since `pretty-quick` does not currently play well with `prettier@^3`, and since we'd like to also use the eslint setup, the pre-commit hook has be changed over to use `lint-staged`. Each monorepo package has its own `lint-staged` configuration based on the files in each repo. Both `eslint` and `prettier` are used as appropriate. If any staged files cannot be auto-fixed by eslint, the commit should fail. Signed-off-by: Scott J Dickerson <[email protected]>
|
Lgtm - thanks for the work on this. |
ibolton336
approved these changes
Aug 29, 2023
gitdallas
approved these changes
Aug 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace the old and broken eslint configuration in
client/package.jsonwith a more robust configuration.eslint,typescript-eslintandprettierare configured to work together to lint javascript and typescript files.Changes:
One eslint config for all packages:
.eslintrc.cjsAdd explicit config files for prettier:
.prettierrc.mjs,.prettierignore, and.editorconfigPrettier v2 and v3 have changes to some styles, including the default for tailing commas. Our configurations are now set to v2 default "es5" trailing commas.
A total of 10 eslint rules have been configurated as warnings instead of errors. There are a lot of these warnings, and fixing them in this PR would create a very large PR. Setting as warnings will allow us to fix the problems and set them back to errors in future PRs. 1 PR for each warn to error conversion is what I anticipate.
A set of errors have been fixed in the PR. These errors only hit a few times each and were easy to fix.
Use
lint-stagedas pre-commit hook:Previously
pretty-quickwas used to runprettier@^2in the client package when committing staged files. Sincepretty-quickdoes not currently play well withprettier@^3, and since we'd like to also use the eslint setup, the pre-commit hook has be changed over to uselint-staged.Each monorepo package has its own
lint-stagedconfiguration based on the files in each repo. Botheslintandprettierare used as appropriate. If any staged files cannot be auto-fixed by eslint, the commit should fail.