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

move triple-quoted string processing into parser #11815

Merged
merged 1 commit into from
Jun 23, 2015
Merged

Conversation

nolta
Copy link
Member

@nolta nolta commented Jun 23, 2015

Strip out triple-quoted string indentation in the parser, instead of using the mstr macro.

Also, no longer treat tabs as 8 spaces. Instead, remove the longest common prefix of spaces and tabs.

Fixes #2682

Strip out triple-quoted string indentation in the parser, instead of
using the mstr macro.

Also, no longer treat tabs as 8 spaces. Instead, remove the longest
common prefix of spaces and tabs.

Fixes #2682
@StefanKarpinski
Copy link
Sponsor Member

:yay:

@ScottPJones
Copy link
Contributor

I wish I knew you were working on this. I ended up wasting time then.

@@ -1027,7 +1027,7 @@
(parse-string-literal s #t)))
(nxt (peek-token s))
(macname (symbol (string #\@ ex '_str)))
(macstr (if (triplequote-string-literal? str) str (cadr str))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo here? This is changing it from picking up the 2nd element of str (cadr str), to picking up the first (car str).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Over terseness can be an impediment to communication. In-line comments explaining the structure that you are processing might have made the change clear (not just for me, but for anybody else who needs to maintain this in the future).

@ScottPJones
Copy link
Contributor

What does this do when there is a there is no common prefix?
The old code in Base.triplequoted didn't look for a prefix, the code I wrote in #11719 doesn't modify the string at all if there is no common prefix (like Python's textwrap.dedent, which seemed to be the inspiration for """ in julia).
How does this treat totally blank lines? The old code ignored them when calculating the leading indentation, I had to treat them specially, so that if they were followed by interpolation, they were still counted when calculating the common prefix, but ignored when not, so things like:

   """
   a
   $var

   b"""

work correctly. The interpolation part may not be an issue, because this code is happening before the code splits the string up into a vector, however the ignoring the indentation of totally blank lines still needs to be handled.
It will be nice to have this in the parser!

@tkelman tkelman added the parser Language parsing and surface syntax label Jun 23, 2015
@tkelman
Copy link
Contributor

tkelman commented Jun 23, 2015

cancelled appveyor, it will probably time out due to #11818 - we can restart the build later if that gets fixed, though there's likely to be a worse-than-usual backlog for the next few days

@nolta
Copy link
Member Author

nolta commented Jun 23, 2015

What does this do when there is a there is no common prefix?

Nothing.

How does this treat totally blank lines?

julia/test/strings.jl

Lines 895 to 900 in 5651bba

@test """
a
b
c
""" == "a$(nl)b$(nl)$(nl)c$(nl)"

@ScottPJones
Copy link
Contributor

What does this do when there is a there is no common prefix?

Nothing.

Good, that was one of the problems I was trying to fix, to make it act like Python's dedent in that case.
Good also about the blank lines... I wasn't ready to tackle fixing the issue in the parser like you did, my Scheme skills are decades rusty... nice to see it handled better.

@StefanKarpinski
Copy link
Sponsor Member

I think this is fine to merge as-is, but I wonder if we shouldn't do those tests by calling parse on strings – it seems quite likely that the test files will get whitespace cleaned at some point, which would rather confusingly break the tests.

StefanKarpinski added a commit that referenced this pull request Jun 23, 2015
move triple-quoted string processing into parser
@StefanKarpinski StefanKarpinski merged commit 1b9fc3d into master Jun 23, 2015
@tkelman tkelman deleted the mn/kill@mstr branch June 24, 2015 03:50
simonster added a commit to JuliaInterop/MATLAB.jl that referenced this pull request Jun 24, 2015
ScottPJones referenced this pull request Jun 27, 2015
Before:

  julia> s = """
             $("\n    ")
             """
  "\n\n"

After:

  julia> s = """
             $("\n    ")
             """
  "\n    \n"

Plus some minor cleanup of 333bb87.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Triple-quoted strings treated an expressions in macros
4 participants