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 #34

Merged
merged 3 commits into from
Feb 19, 2021
Merged

String Interpolation #34

merged 3 commits into from
Feb 19, 2021

Conversation

Licenser
Copy link
Member

@Licenser Licenser commented Dec 2, 2020

@mfelsche
Copy link
Member

mfelsche commented Dec 2, 2020

Great RFC!

Unfortunately I think the current situation doesnt leave too much wiggle room for alternatives here:

A string like:

string::format("this should create a {{}} and say {}", 42) == "this should create a {} and say 42";

cannot really work right now. At runtime, we want to represent the string as compact as possible, a nice little continuous string would the the best option for performance reasons. In the resulting formatted string, we intend {{}} to be {} in the final result, it will be \{\} in the runtime representation, so that string::format doesnt use it as placeholder. When calling string::format we get our literal {} in the resulting formatted string. But what if we do not use string format? We only get a \{\} in the resulting string. What if we use interpolation? "{ 42 } this should work and create a {{}} and say {}" does create a runtime about mismatching count of args and placeholders, error even before we fully executed it.

Lets look at this:

string::format("this { "should work and" } create a {{}} and say {}", 42)

What this transforms into in the AST is:

string::format(string::format("this {} create a \{\} and say {}", "should work and"), 42)

This will obviously fail, as there is only additional argument to the inner string::format although the string somehow produced 2 placeholders. But to the user, this behaviour is very much not obvious.

To make this work as intended the string would need to look like:

string::format("this { "should work and" } create a {{{{}}}} and say {{}}", 42) == "this should work and create a {} and say 42"

Whereas when no interpolation is used, the above raises a runtime error as the number of placeholders and arguments does not match. It needs to be written as:

string::format("this { "should work and" } create a {{}} and say {}", 42) == "this should work and create a {} and say 42"

Having to use different escaping levels depending on whether interpolation is used or not, is not acceptable.

I think the only way to overcome this, is to use option 2. or 3. in the RFC.
While Option 4 seems nice, removing string::format means people cannot do purely dynamic formatting anymore as this cannot be replaced by string interpolation:

string::format(event.format_string, event.args)

Thus, i would vote for adding a prefix like ${} (or any other) to interpolations.

@Licenser Licenser force-pushed the string-interpolation branch from bf4cde0 to c8871fe Compare December 7, 2020 13:00
@Licenser
Copy link
Member Author

Licenser commented Dec 7, 2020

Very good point. And I think you're right. I expanded on the prior art section and added a conclusion section to reflect that input.

@mfelsche mfelsche added the open-review This RFC is undergoing open / public review with intent to proceed label Dec 15, 2020
@Licenser Licenser added the enhancement New feature or request label Dec 18, 2020
@Licenser Licenser requested a review from mfelsche January 29, 2021 13:51
@Licenser Licenser added the active The RFC is active and the implementation is a work in progress label Jan 29, 2021
@Licenser Licenser changed the title String interpolation RFC String interpolation Jan 29, 2021
@darach darach changed the title String interpolation String Interpolation Jan 29, 2021
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
Signed-off-by: Heinz N. Gies <heinz@licenser.net>
@mfelsche mfelsche force-pushed the string-interpolation branch from c8871fe to 85a3ef2 Compare February 19, 2021 13:38
@mfelsche mfelsche removed the open-review This RFC is undergoing open / public review with intent to proceed label Feb 19, 2021
@mfelsche mfelsche merged commit 8fe6409 into main Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
active The RFC is active and the implementation is a work in progress enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants