Skip to content

[CS2] Unicode code point escapes on 2, fixes #4248 #4520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 25, 2017

Conversation

helixbass
Copy link
Collaborator

Fixes #4248: contains the Unicode code point escape handling specific to 2 branch (as suggested by @lydell)

This currently also includes some commits from master (including my code point escape stuff targeted against master ie #4498) that haven't been merged into 2 yet. If it's preferable to separate that out, I can open a pull request for my 2_merged_master branch (against 2 branch), which just contains master merged into 2

But otherwise here is the diff of just my 2-specific changes. Basically we just have to pass around the (he)regex flags in order to be able to distinguish non-u-regexes (whose code point escapes should be rewritten to normal Unicode escapes) from strings/u-regexes (whose code point escapes should not be rewritten)

@lydell
Copy link
Collaborator

lydell commented Apr 21, 2017

@helixbass Could you ping me when the diff is readable? :)

@helixbass
Copy link
Collaborator Author

@lydell sorry I wrongly assumed that once master was merged into 2 the diff would clean up, I re-merged 2 and now it's correct

src/lexer.coffee Outdated
replaceUnicodeCodePointEscapes: (str, options) ->
shouldReplace = options.flags? and options.flags.indexOf('u') is -1
Copy link
Collaborator

Choose a reason for hiding this comment

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

'u' not in options.flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lydell as currently written, options won't always contain a flags key (only does if this is for a regex, in which case flags will be set to the regex flags string) and not in seems to break with an undefined arg. Does seem like there should be a simpler way to write this though - I guess you could do something like options.flags ?= 'u' before 'u' not in options.flags to handle the string case but that seems confusing semantically. Or could refactor to pass in eg dontReplace option instead of flags but it seemed very clean to just pass around existing flags and then have the logic for interpreting it with regards to code point escape replacement be contained in this method

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant only replacing the right side of the and.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lydell ok yup that reads well, updated

@GeoffreyBooth requested that I refactor toJS() into a shared test helper so I opened a pull request #4522 for that (against master). Perhaps wait to merge this one until we've gone through the cycle of merging that into master, merging master into 2, and re-merging 2 here (or it may not matter if that refactor doesn't cause any merge conflicts here)?

@GeoffreyBooth GeoffreyBooth changed the title Iss4248 unicode code point escapes on 2 [CS2] Unicode code point escapes on 2, fixes #4248 Apr 22, 2017
@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 22, 2017
@lydell
Copy link
Collaborator

lydell commented Apr 25, 2017

Unfortunately, there are merge conflicts now :(

@helixbass
Copy link
Collaborator Author

@lydell ok I re-merged 2 and resolved that merge conflict. master still hasn't been merged into 2 since I refactored toJS() to a shared helper via #4522 - it doesn't look like that will cause a merge conflict with this so I guess there's no need to get that merged into 2 before merging this?

@lydell lydell merged commit 7ef5cb4 into jashkenas:2 Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants