-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin)!: [array-type] add "default", "readonly" options #654
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.
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
feat(eslint-plugin)!: [array-type] add "default", "readonly" options #654
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next release will be a breaking change, so let's rework the options completely to make them better.
type ArrayOption = 'array' | 'generic' | 'array-simple';
type Options = [
{
// default case for all arrays
default: ArrayOption,
// optional override for readonly arrays
readonly?: ArrayOption,
},
];
const defaultOptions = [
{
default: 'array',
},
]
This way the user can't provide some weird state of ['generic', { ReadonlyArray: true }
.
type Options = [
{
// default case for all arrays
default: ArrayOption,
// optional override for readonly arrays
readonly?: ArrayOption,
},
]; The new option definition should behave like so // "array-type": ["error", { "default": "array" }]
const x: ReadonlyArray<number> = [1];
const x: Array<number> = [1];
// fixes to
const x: ReadonlyArray<number> = [1];
const x: number[] = [1]; // "array-type": ["error", { "default": "array", "readonly": "array" }]
const x: ReadonlyArray<number> = [1];
const y: Array<number> = [1];
// fixes to
const x: readonly number[] = [1];
const y: number[] = [1]; // "array-type": ["error", { "default": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: readonly number[][] = []; // "array-type": ["error", { "default": "generic", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: ReadonlyArray<Array<number>> = []; Is it right? |
Almost! Sorry, I didn't explain my intention clearly enough: So your examples were almost right. // "array-type": ["error", { "default": "array" }]
const x: ReadonlyArray<number> = [1];
const x: Array<number> = [1];
// fixes to
-const x: ReadonlyArray<number> = [1];
+const x: readonly number[] = [1];
const x: number[] = [1]; // "array-type": ["error", { "default": "array", "readonly": "array" }]
const x: ReadonlyArray<number> = [1];
const y: Array<number> = [1];
// fixes to
const x: readonly number[] = [1];
const y: number[] = [1]; // "array-type": ["error", { "default": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
-const x: readonly number[][] = [];
+const x: ReadonlyArray<Array<number>> = []; // "array-type": ["error", { "default": "generic", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: ReadonlyArray<Array<number>> = []; And of course the situations where the two differ: // "array-type": ["error", { "default": "array", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: string[] = [];
const x: ReadonlyArray<number[]> = []; // "array-type": ["error", { "default": "generic", "readonly": "array" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: readonly Array<number>[] = []; |
@bradzacher Which option can skip check for |
I'm not sure I quite understand - do you mean you were looking for an option to ignore validating readonly arrays completely? I don't think that's a valid state for this rule, as you then can have inconsistent definitions.
The |
It was the main idea to add an option to "array", to have the opportunity to ignore |
I think you've misunderstood me. Which is almost exactly the same as tslints The difference being that tslint will let you use |
223fecd
to
afb22df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to fix two of your tests.
everything else LGTM, great work.
…ipt-eslint into feature/635-add-readonlyArray-option
afb22df
to
5619e25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Codecov Report
@@ Coverage Diff @@
## master #654 +/- ##
==========================================
+ Coverage 94.45% 94.46% +0.01%
==========================================
Files 113 113
Lines 4705 4719 +14
Branches 1288 1297 +9
==========================================
+ Hits 4444 4458 +14
Misses 150 150
Partials 111 111
|
@bradzacher I noticed that the default for this rule is to require By contrast, requiring "@typescript-eslint/array-type": [
"error",
{
default: "array",
readonly: "generic"
}
], What do you think? |
I don't have a problem with recommending the bleeding edge, as the behaviour is documented in the readme. I think it's better to recommend the most consistent syntax, and have people configure the support for their environment if they don't have the new syntax. Yours is the first complaint about it, so it might be uncommon to not be on 3.4? I don't know exactly. Regardless, changing the default option is a breaking change, which means it'd have to wait for a while if we did want to make the change. |
If we measure number of projects, it's probably very uncommon to not be using the latest bleeding edge. Statistically the vast majority of projects are tiny and probably just got started. Whereas if we measure number of dollars earned, older compilers are probably way more likely: Upgrading TypeScript can require nontrivial effort for a production code base. Besides all the files that need fixes, it may also require upgrading |
BREAKING CHANGE: changes config structure
Fixes #635