Skip to content

Use substring instead of substr#20578

Merged
4 commits merged into
masterfrom
substring
Jan 8, 2018
Merged

Use substring instead of substr#20578
4 commits merged into
masterfrom
substring

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Dec 8, 2017

substr takes start and length, substring takes start and end.

Comment thread src/compiler/scanner.ts
return token = SyntaxKind.CommaToken;
case CharacterCodes.dot:
pos++;
if (text.substr(tokenPos, pos + 2) === "...") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How was this not a bug before?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't think we actually do anything with this kind of token in this context. We could just take out this code.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 8, 2018

@sandersn can you take a look.

@sandersn
Copy link
Copy Markdown
Member

sandersn commented Jan 8, 2018

What does this fix? After reading half the changes I don't see uses of substr or substring yet.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 8, 2018

@sandersn It's confusing if you just look at the end result -- original commit just changed substr to substring, then we noticed that this was dead code, so future commits updated type declarations to help detect that.

@ghost ghost merged commit a23bbe6 into master Jan 8, 2018
@ghost ghost deleted the substring branch January 8, 2018 22:54
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
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.

3 participants