Skip to content
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

Implements #3229 - Changed multiline string literals #3246

Merged
merged 5 commits into from
Nov 19, 2013

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented Nov 18, 2013

Implements #3229. Please scrutinize. trimAndEscapeLines might not be the best method name under the sun. All feedback welcome.

@xixixao
Copy link
Contributor Author

xixixao commented Nov 18, 2013

Also implements #610 (duplicates).


# Constructs a string token by escaping quotes and newlines.
makeString: (body, quote, heredoc) ->
return quote + quote unless body
body = body.replace /\\([\s\S])/g, (match, contents) ->
if contents in ['\n', quote] then contents else match
body = body.replace /\\([^])/g, (match, contents) ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep [\s\S]. [^] doesn't work in JScript.

@jashkenas
Copy link
Owner

Fantastic. This works equally well for strings with interpolations in them?

Perhaps formatString instead of trimAndEscapeLines?

@xixixao
Copy link
Contributor Author

xixixao commented Nov 18, 2013

@jashkenas See the last test case, any additional formats to test for?

@xixixao
Copy link
Contributor Author

xixixao commented Nov 18, 2013

I added some maybe too esoteric cases (let me know if that's overdoing it).

@@ -18,6 +18,46 @@ eq "four five", 'four

five'

test "#3229, multine strings", ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: "multine" instead of "multiline"

@lydell
Copy link
Collaborator

lydell commented Nov 18, 2013

Tests I feel are missing:

# Handle escaped backslashes correctly.
eq 'escaped backslash at EOL\\
    next line', 'escaped backslash at EOL\\ next line'
eq 'triple backslash\\\
    next line', 'triple backslash\\next line'

# Another way of preserving whitespace around line endings.
eq 'first line
    \   backslash at BOL', 'first line \   backslash at BOL'
eq 'first line\
    \   backslash at BOL', 'first line\   backslash at BOL'

# Edge case.
eq 'lone

        \

      backslash', 'lone backslash'

Or something like that.

@xixixao
Copy link
Contributor Author

xixixao commented Nov 18, 2013

@lydell The "Another way of..." cases don't make sense to me, did you mean not to put the backslash in the formatted strings?

@xixixao
Copy link
Contributor Author

xixixao commented Nov 19, 2013

@lydell If not, we have that behavior (all your test cases are now included).

jashkenas added a commit that referenced this pull request Nov 19, 2013
Implements #3229 - Changed multiline string literals
@jashkenas jashkenas merged commit 6847400 into jashkenas:master Nov 19, 2013
@lydell
Copy link
Collaborator

lydell commented Nov 19, 2013

I did mean to put the backslash in the formatted strings. Even though we don't do anything special to the backslashes in that case, the tests are still important: We must make sure that way of preserving whitespace around line endings really works. Tests also work a bit as documentation.

Looks like I missed one test case: Even moar backslashes. Infinitely many should work. I tried this on master:

'\\\\\\
  a'

Expected:

'\\\\\\ a';

Actual:

'\\\\\a';

@xixixao
Copy link
Contributor Author

xixixao commented Nov 19, 2013

Ah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants