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.

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

use typescript-eslint array-type rule #7974

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Feb 24, 2019

Summary

The TypeScript sources currently use Array<T> and T[] inconsistently.
This makes it harder to grep for e.g. Array<Test> or Test[] because you might miss some of them that use the other syntax.
It might also confuse TS beginners who wonder what the difference between the two might be.
eslint-typescript has the array-type rule to enforce one syntax or the other (or alternatively [] for simple types and Array for complex ones).
I've set it to 'array' ([]) and run the auto fixer, but I'd be open to discussion / links to convince me why another option would be better ;)

Test plan

@SimenB
Copy link
Member

SimenB commented Feb 24, 2019

@cpojer expressed a preference for Array<> in #6380 (comment). I personally don't care, except I dislike parens (e.g. (string | boolean)[] - I find Array<string | boolean> more readable).

Huge +1 to consistency though, and an autofixable rule helps.

@cpojer @thymikee any preference?

@jeysal
Copy link
Contributor Author

jeysal commented Feb 24, 2019

except I dislike parens (e.g. (string | boolean)[] - I find Array<string | boolean> more readable).

I guess that's why they added array-simple - to allow using [] in cases where it is more readable and Array for complex types. tslint:recommended uses this option. It'd be a smaller win on consistency though.

@cpojer
Copy link
Member

cpojer commented Feb 25, 2019

Heavy preference for Array<…> because it is consistent with any other generic type. There is no reason that Arrays should be special and if the only argument is that it's shorter to type, I don't think that's a good reason.

In Flow, we use square brackets when we mean tuples instead of Arrays, but I'm not sure if TypeScript supports it.

I'm not gonna block you but I'd like to hear a good reason over why array generics are special compared to any other generic type.

@jeysal
Copy link
Contributor Author

jeysal commented Feb 25, 2019

Well, JS has special syntax for initializing arrays so IMO it makes sense that there's also a special syntax for declaring their types, and it's particularly familiar because it uses the same characters [].
But I don't want to bikeshed on this too much, I just picked this particular lint setting because it's what I've seen most commonly used in TypeScript (whereas most Flow users afaik use Array).
We can give it a day or so to see if anyone else has a huge disdain for either syntax and otherwise I'll change it to your preference.

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

Another point - I read (string | boolean)[] as "string or boolean array" whilst Array<string | boolean> is "array of string or boolean". It also matches other generics (so same as Set<string> or arguments to functions). And since it's autofixable, I guess most people could write whatever they want and have eslint fix it? I'm not sure how normal "fix on save" is these days

@jeysal
Copy link
Contributor Author

jeysal commented Feb 25, 2019

Yeah it being autofixable is why I don't really feel the need to convince anyone 😄
What do you mean by "string or boolean array" vs "array of string or boolean" though? "string or boolean array" vs "string array or boolean array" would matter, but where's the difference between those two?

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

I mean that I read it in a way that does not represent what it is - I hit the "modifier" at the end which changes the semantics of the expression instead of it reading correctly the first time through.

"this take a string or a boolean, as an array". Also "string or boolean array" - is that string | boolean[] or (string | boolean)[]?

@jeysal
Copy link
Contributor Author

jeysal commented Feb 25, 2019

Ah, got it 👍

@jeysal
Copy link
Contributor Author

jeysal commented Feb 28, 2019

Updated with generic (Array<T>) instead of array (T[])

@SimenB SimenB merged commit 72d01cc into jestjs:master Feb 28, 2019
@jeysal jeysal deleted the eslint-array-type branch March 1, 2019 08:48
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants