Prettier and eslint improvements (no dependency upgrades yet) #574
Conversation
mattpocock
commented
Aug 20, 2024
•
mattpocock
commented
- Make prettier run on the entire repo, not just the source files
- Remove prettier from eslint configs
- Made the CI check that code is formatted correctly
|
mattpocock
changed the title
| @@ -41,8 +42,6 @@ | |||
| "@typescript-eslint/parser": "4.28.1", | |||
| "babel-jest": "27.5.1", | |||
| "eslint": "7.30.0", | |||
| "eslint-config-prettier": "7.1.0", | |||
There was a problem hiding this comment.
[Non-blocker] Is there a reason to avoid eslint-config-prettier? I thought it's meant to prevent conflicting rules
There was a problem hiding this comment.
There was a problem hiding this comment.
I also agree with removing eslint-config-prettier and the prettier docs advise to essentially do the same (i.e. let linters lint, and formatters format)
package.json
Outdated
| "lint": "eslint ./src --ext .ts", | ||
| "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix", | ||
| "format": "prettier --write . && npm run lint -- --fix", | ||
| "format-check": "prettier --check .", |
There was a problem hiding this comment.
Lets utilize prettier and eslint's cacheing options
- https://prettier.io/docs/en/cli#--cache
- https://eslint.org/docs/latest/use/command-line-interface#caching
| "lint": "eslint ./src --ext .ts", | |
| "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix", | |
| "format": "prettier --write . && npm run lint -- --fix", | |
| "format-check": "prettier --check .", | |
| "lint": "eslint ./src --ext .ts --cache --cache-location 'node_modules/.cache/eslintcache", | |
| "format": "prettier --write --cache --cache-location='node_modules/.cache/prettiercache' . && npm run lint -- --fix", | |
| "format-check": "prettier --check --cache --cache-location='node_modules/.cache/prettiercache' .", |
There was a problem hiding this comment.
Getting this warning from Prettier:
[warn] Ignored unknown option --cache.
[warn] Ignored unknown option --cache-location=node_modules/.cache/prettiercache.
| @@ -41,8 +42,6 @@ | |||
| "@typescript-eslint/parser": "4.28.1", | |||
| "babel-jest": "27.5.1", | |||
| "eslint": "7.30.0", | |||
| "eslint-config-prettier": "7.1.0", | |||
There was a problem hiding this comment.
I also agree with removing eslint-config-prettier and the prettier docs advise to essentially do the same (i.e. let linters lint, and formatters format)
There was a problem hiding this comment.
lgtm 👍 last things:
- let's just make sure we make the one change as per @dmmulroy's comment
- Resolve merge conflicts
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
tests/index.test.ts
Outdated
| type ExpecteResult = Result<[ string, number, boolean ], (string | number | boolean)[]> | ||
| const heterogenousList: HeterogenousList = [okAsync('Yooooo'), okAsync(123), okAsync(true)] | ||
|
|
||
| type ExpecteResult = Result<[string, number, boolean], [string, number, boolean]> |
There was a problem hiding this comment.
@mattpocock Isn't that the reason tests are failing?
There was a problem hiding this comment.
Apparently not intentional, let me check
There was a problem hiding this comment.
Looks like I was pulling from origin/master instead of upstream/master, my bad. Fixed.
…' of https://github.com/mattpocock/neverthrow into matt/prettier-and-eslint-improvements--no-upgrades-yet-
|
This PR is getting a bit out of hand. I am having real trouble rebasing every time a new change is merged. I'll create a new PR which adds the ability to run prettier, but doesn't format the whole repo. Once that's merged we can format the repo properly. |
