Skip to content

Create separate constructors for Tokens and Identifiers#9529

Merged
sheetalkamat merged 6 commits into
masterfrom
identifierAndTokenConstructor
Jul 14, 2016
Merged

Create separate constructors for Tokens and Identifiers#9529
sheetalkamat merged 6 commits into
masterfrom
identifierAndTokenConstructor

Conversation

@sheetalkamat
Copy link
Copy Markdown
Member

No description provided.

Comment thread src/compiler/types.ts Outdated
flags: NodeFlags;
}

export interface Token extends Node { }
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Jul 6, 2016

Choose a reason for hiding this comment

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

Consider a __tokenTag

Comment thread src/services/services.ts Outdated
public jsDocComments: JSDocComment[];
public __tokenTag: any;

constructor(public pos: number, public end: number) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you list all assignments to properties in the same order as in the NodeObject ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They are in same order as node object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i understand. it just took me a bit to verify that by looking at the ones done in the constructor explicitly and the ones done because they are parameter properties.

my comment is to not use parameter properties just to make it clear what the order is.

also worth adding a comment that these need to stay in sync on all three classes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure will do.

Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Jul 11, 2016

Choose a reason for hiding this comment

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

Sorry, that's my fault. :)

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jul 14, 2016

👍

@sheetalkamat sheetalkamat merged commit f16f276 into master Jul 14, 2016
@sheetalkamat sheetalkamat deleted the identifierAndTokenConstructor branch July 14, 2016 20:35
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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