Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add array-simple option to array-type rule #1539

Merged
merged 3 commits into from
Sep 26, 2016

Conversation

ScottSWu
Copy link
Contributor

@ScottSWu ScottSWu commented Sep 5, 2016

Only use '[]' notation for 'simple' types.

Fixes #1526

type: "style",
};
/* tslint:enable:object-literal-sort-keys */

public static FAILURE_STRING_ARRAY = "Array type using 'Array<T>' is forbidden. Use 'T[]' instead.";
public static FAILURE_STRING_GENERIC = "Array type using 'T[]' is forbidden. Use 'Array<T>' instead.";
public static FAILURE_STRING_ARRAY_SIMPLE = "Array type using 'Array<T>' is forbidden for simple types. Use 'T[]' instead.";
public static FAILURE_STRING_GENERIC_SIMPLE = "Array type using 'T[]' is forbidden for simple types. Use 'Array<T>' instead.";
Copy link
Contributor

Choose a reason for hiding this comment

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

this one should say "for non-simple types"

@alexeagle
Copy link
Contributor

thanks, Scott! can't wait to tsunami this one again.

@jkillian
Copy link
Contributor

Thanks for reviewing this @alexeagle! Will merge after corrections

@alexeagle
Copy link
Contributor

Just ran this fix across google's codebase.

I noticed that
Array<FilterItem<T>> => FilterItem<T>[] which I think we don't mean to be a simple type?
especially in a case like
DataValue<Array<DataValue<any>>> => DataValue<DataValue<any>[]>
where the Array is just one of several collection generics, and it gets such different treatment.

@ScottSWu
Copy link
Contributor Author

Hmm, I marked all TypeReferences, so I'm guessing it should only be TypeReferences that are not generics.

In that case, should Array<Array<number> still be number[][]?

@alexeagle
Copy link
Contributor

number[][] seems like the right representation, but then what about if I have a List type in my app?

List<number[]> vs List<Array<number>>
and
List<number>[] vs. Array<List<number>>

If we simply apply to non-generic TypeReferences we get
Array<number[]> (note you'd have to be aware that number[] is actually a generic)
List<number[]>
Array<List<number>>

I think the real readability issue is that the syntax sugar is postfix, so if it follows a very long expression, like
let longThing: MultiMap<Set<A>, NonIntersectingPair<B>, Comparator<A, B>>[];
You would have liked to read the Array bit at the beginning because by the time you encounter it, you think you've already understood the type.

So on that grounds, I think treating generics as a "non-simple" type is the right thing to do, even if we lose a bit of sugar for number[][]

@alexeagle
Copy link
Contributor

@evmar comments?

@jkillian
Copy link
Contributor

If I had to vote in the cases you gave above, I prefer the following (bolded) options:

Array<Array<number> vs number[][]
List<number[]> vs List<Array<number>>
List<number>[] vs. Array<List<number>>

@alexeagle
Copy link
Contributor

@ScottSWu would you like me to take this one over?

Only use '[]' notation for 'simple' types.
* Nested Arrays are ok as long as their type argument is also simple
* Add namespace test case
* Fix typos and test cases
@ScottSWu
Copy link
Contributor Author

Sorry - PTAL

The new commit follows @jkillian's preferences with number[][]. A type is simple if it is non-generic or is an Array with another simple type argument.

@alexeagle
Copy link
Contributor

Thanks @ScottSWu I ran it across all our code again. Looks really good!

Found bugs:
1)
pointsLineData as any as[number, number][] became pointsLineData as any asArray<[number, number]> (syntax error)

interface EAP {
  '.b.SpawnInfo.spawnInfo': {argument: string[];};
}

became

interface EAP {
  '.b.SpawnInfo.spawnInfo': {argument: [];};
[]

@ScottSWu
Copy link
Contributor Author

The first is fixable, but the second looks weird - I'll add that as a test case, but could that be a syncing issue?

@alexeagle
Copy link
Contributor

I doubt it's sync in this case (I own the code that was mangled and haven't changed it) but as long as there's a test case, tslint is not responsible for what happens :)

* Add test cases for interface and 'as' keyword
* Fix format in test files
@ScottSWu
Copy link
Contributor Author

Added both test cases. For the first bug, I added a space for the as keyword only - couldn't think of other situations where a type follows an alphanumeric character.

@alexeagle
Copy link
Contributor

LGTM thanks a lot Scott! Looking forward to turning this one on.

@alexeagle
Copy link
Contributor

ping @jkillian for LGTM

@adidahiya adidahiya merged commit 1e3a428 into palantir:master Sep 26, 2016
@adidahiya
Copy link
Contributor

thanks @ScottSWu!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants