Skip to content

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Dec 21, 2015

Fix #4867

Copy link
Member

Choose a reason for hiding this comment

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

I liked the short form better, although putting it in its own function is nice.

@sandersn
Copy link
Member

I don't see the tests yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment why do we need to unescape the text here

Copy link
Member

Choose a reason for hiding this comment

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

prefixedWithUnderscores

Copy link
Member

Choose a reason for hiding this comment

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

Below we use identifier.length >= 3 so make these align.

@yuit
Copy link
Contributor Author

yuit commented Jan 13, 2016

@DanielRosenwasser @sandersn @vladima any comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of changing the comparison from identifier.length >= 2 to identifier.length >= 3? With this change, we won't escape __a to ___a. Is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rbuckton for pointing this out. we don't really need the check any more since we will unescape anyway

@rbuckton
Copy link
Contributor

👍

@sandersn
Copy link
Member

:+!:

yuit added a commit that referenced this pull request Jan 27, 2016
Fix incorrectly emitting underscore of imported property
@yuit yuit merged commit 265069e into master Jan 27, 2016
@yuit yuit deleted the fix4867_transpiling branch January 27, 2016 00:04
@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.

6 participants