Skip to content

Remove template strings in checker.ts#18016

Merged
2 commits merged into
masterfrom
rm_template
Aug 28, 2017
Merged

Remove template strings in checker.ts#18016
2 commits merged into
masterfrom
rm_template

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Aug 24, 2017

In the chrome debugger, the presence of a template string causes the rest of the document to be highlighted in red.
These aren't providing a lot of value vs string concatenation, so it seems easier to just remove them.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Aug 24, 2017

did you file an issue for the chrome ?

@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 24, 2017

I think there was one already, but the bug tracker is down right now.

@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 25, 2017

Found it: https://bugs.chromium.org/p/chromium/issues/detail?id=667578

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Given that the bug has been sitting on Chrome for 9+ months and that you can't hover variable values without it being fixed, I'd say this change is worthwhile.

Comment thread src/compiler/checker.ts Outdated
}
}

function enquote(s: string): string {
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.

quote is a fine name in my opinion. It's not ambiguous in our codebase and English verbs nouns without a required prefix*.

*Usually.

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.

I'm not sure two uses justifies the existence of this function actually. Your call.

@weswigham
Copy link
Copy Markdown
Member

The chrome issue is actually likely an issue with the code mirror grammar they use for JS/TS. (Which, as an aside, probably needs to be updated for newer TS syntax)

@ghost ghost merged commit 934da9f into master Aug 28, 2017
@ghost ghost deleted the rm_template branch August 28, 2017 22:03
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

4 participants