Skip to content

Handle windows linebreaks in getSourceFileImportLocation#19791

Merged
amcasey merged 4 commits into
microsoft:masterfrom
amcasey:ImportLineBreaks
Nov 7, 2017
Merged

Handle windows linebreaks in getSourceFileImportLocation#19791
amcasey merged 4 commits into
microsoft:masterfrom
amcasey:ImportLineBreaks

Conversation

@amcasey
Copy link
Copy Markdown
Member

@amcasey amcasey commented Nov 7, 2017

No description provided.

@amcasey amcasey requested a review from weswigham November 7, 2017 02:31
Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Some style comments.

Comment thread src/services/utilities.ts Outdated
return position;

function AdvancePastLineBreak() {
if (text.charCodeAt(position) === 0xD) {
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.

ts.characterCodes.carriageReturn over 0xD?

Comment thread src/services/utilities.ts Outdated
position++;
}

if (text.charCodeAt(position) === 0xA) {
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.

Same here, I think it's more clear if the character codes enum is used.

Comment thread src/services/utilities.ts Outdated
return position;

function AdvancePastLineBreak() {
if (text.charCodeAt(position) === 0xD) {
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.

CharacterCodes.carriageReturn

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.

Nifty.

Comment thread src/services/utilities.ts Outdated
position++;
}

if (text.charCodeAt(position) === 0xA) {
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.

Charactercodes.lineFeed

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.

use isLineBreak to handle other line break characters like \u2028 ans 29

Comment thread src/services/utilities.ts Outdated
}
return position;

function AdvancePastLineBreak() {
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.

would skipTrivia be sufficient here?

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.

Wouldn't that skip other whitespace and comments?

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.

Though I am confused as to why this is upper camel case instead of lower camel case like every other helper function .

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.

That would be because I'm a C# dev. 😉

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.

but you are at the end of the line anyways.. so comments and white spaces are not an issue here..

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.

Good point. I notice, though, that skipTrivia doesn't handle the other linebreak characters. Unless you object, I'll leave it like this for the sake of explicitness.

@amcasey amcasey merged commit 62eeb72 into microsoft:master Nov 7, 2017
@amcasey amcasey deleted the ImportLineBreaks branch November 7, 2017 19:34
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

3 participants