Skip to content

refactor: use type guard instead of unnecessary or incorrect type assertions#659

Merged
mgechev merged 1 commit intomgechev:masterfrom
rafaelss95:refactor-type-guard
Jun 12, 2018
Merged

refactor: use type guard instead of unnecessary or incorrect type assertions#659
mgechev merged 1 commit intomgechev:masterfrom
rafaelss95:refactor-type-guard

Conversation

@rafaelss95
Copy link
Collaborator

This:

  • Replaces a lot of any usages with more expressive types;
  • Uses type guard instead of (unnecessary) type casts;

PS: You can see that there are a lot more places that could be changed, but these or I can't replace, because changing them, breaks something or unfortunately it has to be any.

logger.error('Cannot parse the styles of', ((<any>metadata.controller || {}).name || {}).text, e);
const {
controller: { name }
} = metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs defaults in order to guard the same way the original did. Not sure if intentional.

Copy link
Collaborator Author

@rafaelss95 rafaelss95 Jun 3, 2018

Choose a reason for hiding this comment

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

The property controller always exists according to:

export class DirectiveMetadata {
controller!: ts.ClassDeclaration;
decorator!: ts.Decorator;
selector!: string;
}

... and it makes sense, because once we instantiate a DirectiveMetadata, we pass all properties:

return Object.assign(new DirectiveMetadata(), {
controller: d,
decorator: dec,
selector: selector.unwrap()
});

... now I'm curious on why we are using Object.assign instead of a trivial constructor. Thoughts?


Anyway, these "defaults" should have been changed in #631, but I really forgot about it. 😞

@rafaelss95 rafaelss95 requested review from mgechev and wKoza June 6, 2018 22:46
@rafaelss95
Copy link
Collaborator Author

bump 👽

Copy link
Collaborator

@wKoza wKoza left a comment

Choose a reason for hiding this comment

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

LGTM

@wKoza wKoza requested review from mgechev and removed request for mgechev June 12, 2018 09:11
private consumer: SourceMapConsumer;

constructor(sourceFile: ts.SourceFile, options: IOptions, protected codeWithMap: CodeWithSourceMap, protected basePosition: number) {
constructor(sourceFile: ts.SourceFile, options: IOptions, public codeWithMap: CodeWithSourceMap, protected basePosition: number) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not protected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was necessary because I've changed the type of context in trackByRule#35 from any to BasicTemplateAstVisitor (https://github.com/mgechev/codelyzer/pull/659/files#diff-26d9296d5e0a25d3fe6a9b174e67da35R35)... it gives an error with protected:

src/trackByFunctionRule.ts(44,30): error TS2446: Property 'codeWithMap' is protected and only accessible through an instance of class 'TrackByFunctionTemplateVisitor'.

import { SourceMappingVisitor } from '../sourceMappingVisitor';

const getExpressionDisplacement = (binding: any) => {
const getExpressionDisplacement = (binding: ast.TemplateAst) => {
Copy link
Owner

Choose a reason for hiding this comment

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

❤️

@mgechev mgechev merged commit 6dab8d7 into mgechev:master Jun 12, 2018
@rafaelss95 rafaelss95 deleted the refactor-type-guard branch June 12, 2018 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants