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.

Sign up for GitHub

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

Jump to bottom

feat: Add rule types #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: Add rule types #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01c8daf
feat: Add rule types
nzakas Aug 20, 2024
dcc43ad
Remove typescript-eslint stylistic rules
nzakas Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default [
// TypeScript
...tseslint.config({
files: ["**/*.ts"],
extends: [...tseslint.configs.strict, ...tseslint.configs.stylistic],
extends: [...tseslint.configs.strict],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 not a fan of the stylistic config? Any reason in particular to remove this?

From running locally, I see two complaints:

/Users/josh/repos/rewrite/packages/core/src/types.ts
  108:8   error  A record is preferred over an index signature                                        @typescript-eslint/consistent-indexed-object-style
  389:12  error  Array type using 'Array<SuggestedEdit>' is forbidden. Use 'SuggestedEdit[]' instead  @typescript-eslint/array-type
  • @typescript-eslint/array-type: purely stylistic for consistency. If you prefer Array<...> that's available as an option.
  • @typescript-eslint/consistent-indexed-object-style: it's meant to be purely stylistic, but there are some changes between interfaces and records. If you want to use catch-all string signatures the way RuleVisitor is set up now, I'd just disable the rule with a comment. Either inline or in the config altogether.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I didn't think either of these warnings were helpful, and I didn't feel like fighting other rules that might come up (generally not a fan of applying rules blindly, especially ones that just seem to be enforcing stylistic preferences that I don't have).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. My (very obviously biased as typescript-eslint maintainer) recommendation would be to keep them enabled, but I understand why you'd want to avoid the stylistic preferences.

The terminology here is unfortunate: "stylistic" as used by that shareable config encompasses more than what "stylistic" means in ESLint core. It contains more than just consistency rules; it also has some real best practice violations and checks for likely bug causing syntax.

I'd recommend at least considering leaving the following rules (here or as a followup issue) that most folks see as definitive positives beyond just stylistic consistency:

...and similarly at least considering these rules on "just pick one and stick with it" consistency points that, in my experience, tend to get annoyingly split in a repo over time:

https://typescript-eslint.io/rules/?=stylistic-xtypeInformation has the full listing of rules in tseslint.configs.stylistic.

rules: {
"no-use-before-define": "off",
},
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
},
"homepage": "https://github.com/eslint/rewrite#readme",
"devDependencies": {
"json-schema": "^0.4.0",
"typescript": "^5.4.5"
},
"engines": {
Expand Down
Loading