lint: use gitignore, enforce prettier rules #5307
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
justinmk3
commented
Jul 12, 2024
| @@ -16,7 +16,7 @@ | |||
| "singleQuote": true, | |||
| "semi": false, | |||
| "bracketSpacing": true, | |||
| "arrowParens": "avoid", | |||
| "arrowParens": "always", | |||
There was a problem hiding this comment.
Partial solution work for #3662
There was a problem hiding this comment.
How does this partially solve the issue? Seems like arrowParens only adds parenthesis around the arguments, but the issue is relating to how arrow funcs structure their returns.
justinmk3
commented
Jul 12, 2024
| @@ -32,10 +32,8 @@ | |||
| "package": "npm run package -w packages/toolkit -w packages/amazonq", | |||
| "newChange": "echo 'Must specify subproject/workspace with -w packages/<subproject>' && false", | |||
| "createRelease": "echo 'Must specify subproject/workspace with -w packages/<subproject>' && false", | |||
| "format": "prettier --check plugins && npm run format -w packages/ --if-present", | |||
| "formatfix": "prettier --write plugins && npm run formatfix -w packages/ --if-present", | |||
There was a problem hiding this comment.
Removed all format and formatfix tasks. They're redundant now that eslint enforces prettier rules.
justinmk3
commented
Jul 12, 2024
| "lint": "npm run lint -w packages/ --if-present && npm run format", | ||
| "lintfix": "eslint -c .eslintrc.js --fix --ext .ts packages plugins && npm run formatfix", | ||
| "lint": "npm run lint -w packages/ --if-present", | ||
| "lintfix": "eslint -c .eslintrc.js --ignore-path .gitignore --ignore-pattern '**/*.json' --ignore-pattern '**/*.gen.ts' --ignore-pattern '**/types/*.d.ts' --ignore-pattern '**/src/testFixtures/**' --ignore-pattern '**/resources/js/graphStateMachine.js' --fix --ext .ts packages plugins", |
There was a problem hiding this comment.
Future: need to extract the eslint invocation to something like scripts/lint.ts.
justinmk3
commented
Jul 12, 2024
| ], | ||
| { | ||
| throws: false, | ||
| } | ||
| ) | ||
| assert.strictEqual(result.status, 0, result.stdout.toString()) | ||
| assert.strictEqual(result.status, 0, result.output.toString()) |
There was a problem hiding this comment.
stderr wasn't shown
justinmk3
commented
Jul 12, 2024
Comment on lines
+34
to
+38
| '../amazonq', | ||
| '../core', | ||
| '../toolkit', | ||
| // TODO: fix lint issues in scripts/ | ||
| // '../../scripts', |
There was a problem hiding this comment.
run eslint on all packages.
Future: testLint/ should live outside of core/
justinmk3
commented
Jul 12, 2024
Comment on lines
+18
to
+23
| // Note: eslint currently does not support multiple --ignore-path args. | ||
| // Use --ignore-pattern as a workaround. | ||
| '--ignore-path', | ||
| '../../.eslintignore', | ||
| '../../.gitignore', | ||
| '--ignore-pattern', | ||
| '**/*.json', |
There was a problem hiding this comment.
re-use gitignore instead of eslintignore
|
/runintegrationtests |
justinmk3
force-pushed
the
lint
branch
2 times, most recently
from
d9848fd to
6e49f31
Compare
Problem:
javascript arrow functions allow braces `{ ... }` to surround an
expression without a `return`, which leads to bugs such as
aws#3659
aws#3662
Example:
return supplementalContexts.filter(item => { item.content.trim().length !== 0 })
should be:
return supplementalContexts.filter(item => item.content.trim().length !== 0)
Solution:
- Set prettier `arrowParens=always`. This is the default, explained
here: https://prettier.io/docs/en/options.html#arrow-function-parentheses
> At first glance, avoiding parentheses may look like a better choice
because of less visual noise. However, when Prettier removes
parentheses, it becomes harder to add type annotations, extra
arguments or default values as well as making other changes.
Consistent use of parentheses provides a better developer experience
when editing real codebases, which justifies the default value for the
option.
- Note: this is equivalent to the eslint `@stylistic/no-confusing-arrow` rule.
- https://eslint.style/packages/default#stylistic-eslint-plugin
- https://eslint.style/rules/default/no-confusing-arrow
This task was added as workaround. It's not need now because we have "eslint-plugin-prettier" installed which tells eslint to include rules decided by our prettier config.
Since our eslint rules now are informed by our prettier config (performed by the "eslint-plugin-prettier" package), `prettier --write` is unnecessary because it's implictly done by `eslint --fix`. https://github.com/prettier/eslint-plugin-prettier
TODO: extract the eslint invocation to scripts/lint.ts so that it's not duplicated in the "lintfix" task.
hayemaxi
approved these changes
Jul 12, 2024
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.
Problem
Solution
eslint --ignore-path .gitignoreand remove .eslintignoreeslint-plugin-prettierwhich automatically enforces prettier rules as eslint rules.Note: the formatting changes are from
arrowParens=always. See commit for details.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.