This repository has been archived by the owner on Mar 25, 2021. It is now read-only.
Clean up array-type and consider 'object' as a simple type #2353
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
ajafff
approved these changes
Mar 24, 2017
src/rules/arrayTypeRule.ts
Outdated
| @@ -51,83 +53,116 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
| public static FAILURE_STRING_GENERIC_SIMPLE = "Array type using 'T[]' is forbidden for non-simple types. Use 'Array<T>' instead."; | |||
|
|
|||
| public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | |||
| const alignWalker = new ArrayTypeWalker(sourceFile, this.getOptions()); | |||
| return this.applyWithWalker(alignWalker); | |||
| const option = this.getOptions().ruleArguments[0]; | |||
src/rules/arrayTypeRule.ts
Outdated
| return true; | ||
| } else { | ||
| case 1: | ||
| return utils.isIdentifier(typeName) && typeName.text === "Array" && isSimpleType(typeArguments[0]); |
There was a problem hiding this comment.
use typeName.kind === ts.SyntaxKind.Identifier instead of isIdentifier. typeName is a discriminated union so this narrows the type.
src/rules/arrayTypeRule.ts
Outdated
| const OPTION_GENERIC = "generic"; | ||
| const OPTION_ARRAY_SIMPLE = "array-simple"; | ||
| type Option = "array" | "generic" | "array-simple"; | ||
| const OPTION_ARRAY: Option = "array"; |
There was a problem hiding this comment.
no need for the type annotation
src/rules/arrayTypeRule.ts
Outdated
| const alignWalker = new ArrayTypeWalker(sourceFile, this.getOptions()); | ||
| return this.applyWithWalker(alignWalker); | ||
| const option = this.getOptions().ruleArguments[0]; | ||
| return this.applyWithFunction(sourceFile, (ctx) => walk(ctx, option)); |
There was a problem hiding this comment.
option can be passed as 3rd parameter and destructured inside walk
ajafff
reviewed
Mar 24, 2017
| function checkTypeReference(node: ts.TypeReferenceNode): void { | ||
| const { typeName, typeArguments } = node; | ||
|
|
||
| if (option === "generic" || !isArrayIdentifier(typeName)) { |
There was a problem hiding this comment.
to be consistent. you should use the constants. e.g. OPTION_GENERIC here and in all other comparisons below
There was a problem hiding this comment.
option is a string union, so any misspellings would be caught. It's like an enum.
|
@andy-hanson could you please resolve the new merge conflicts? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
PR checklist
Overview of change:
Cleaned up the implementation and added 'object' to the list of simple types.