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

String interpolation: combine contiguous string literals #8581

Merged
merged 1 commit into from Dec 13, 2019

Conversation

@asterite
Copy link
Member

asterite commented Dec 13, 2019

Fixes #8576

I think this should go in 0.32.1 because this was an accidental breaking change.

The fix is done at normalization because doing it before breaks the formatter a bit (mainly around string literals that aren't actually string literals, like __FILE__), but this way is fine.

Copy link
Contributor

Sija left a comment

Just as a side-note - it doesn't actually fix #8576, it just pushes the limit bit more but the root cause is still there.

@asterite asterite force-pushed the bug/combine-contiguous-string-literals branch from 3745a02 to 695f10b Dec 13, 2019
@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Dec 13, 2019

@Sija I just don't think a heredoc with ~150 things interpolated in them is realistic. We can close that one, and if someone stumbles upon it again with a real code with that many interpolations we can tackle it too.

@vlazar

This comment has been minimized.

Copy link
Contributor

vlazar commented Dec 13, 2019

The heredoc in issue #8576 seem to have just 1 interpolation inside and many more lines. The use-case was real, a code generation in https://github.com/cubos/sdkgen/blob/master/src/target/java_android.cr

I think it's a rare use-case, but I imagine in some cases having a long source inside heredoc with some interpolations inside would look nicer that splitting generated source into multiple heredocs.

So if there is a 1000 lines long text inside heredoc and a couple of interpolations inside, will it work now with this fix?

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Dec 13, 2019

Yes, that case will work. The bug was that a string literal was generated for every line, regardless of interpolation. Now the size of the tuple depends on the number of interpolations.

@lbguilherme

This comment has been minimized.

Copy link
Contributor

lbguilherme commented Dec 13, 2019

Out of curiosity: Why does the parser generate a string interpolation for every line instead of generating a multi-line string literal?

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Dec 13, 2019

If I remember correctly the parser will spit out lines because then it needs to realign each of them according to the heredocs indentation. I tried to fix it in the parser, it broke the formatter and so I made the fix later on.

In any case, once this is merged I will maybe send another PR in which constants that refer to strings and appear in string interpolations will be inlined right away, so contiguous merging will still be needed.

@bcardiff bcardiff added this to the 0.32.1 milestone Dec 13, 2019
@asterite asterite merged commit b91c698 into master Dec 13, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite deleted the bug/combine-contiguous-string-literals branch Dec 13, 2019
sthagen added a commit to sthagen/crystal that referenced this pull request Dec 13, 2019
String interpolation: combine contiguous string literals (crystal-lang#8581)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.