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

Remove string concat in favor of a flat list #1799

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

kddnewton
Copy link
Collaborator

Right now when you have a lot of string concats it ends up being difficult to work with because of the depth of the tree. You end up descending very far for every string literal that is part of the concat.

There are already times when we use an interpolated string node to group together two string segments that are part of the same string (like when they are interupted by the contents of a heredoc). This commit takes the same approach and replaces string concats with interpolated string nodes.

Now that they're a flat list, they should be much easier to work with. There's still some missing information here that would be useful to consumers: whether or not there is actually any interpolation contained in the list. We could remedy this with another node type that is named something like string list, or we could add a flag to interpolated string node indicating that there is interpolation. Either way I want to solve that in a follow-up commit, since this commit is valuable on its own.

Fixes #1479

Right now when you have a lot of string concats it ends up being
difficult to work with because of the depth of the tree. You end
up descending very far for every string literal that is part of the
concat.

There are already times when we use an interpolated string node to
group together two string segments that are part of the same string
(like when they are interupted by the contents of a heredoc). This
commit takes the same approach and replaces string concats with
interpolated string nodes.

Now that they're a flat list, they should be much easier to work
with. There's still some missing information here that would be
useful to consumers: whether or not there is _actually_ any
interpolation contained in the list. We could remedy this with
another node type that is named something like string list, or we
could add a flag to interpolated string node indicating that there
is interpolation. Either way I want to solve that in a follow-up
commit, since this commit is valuable on its own.
@kddnewton kddnewton merged commit b2234d8 into main Nov 21, 2023
45 of 46 checks passed
@kddnewton kddnewton deleted the remove-string-concat branch November 21, 2023 16:36
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.

Consider combining "foo" "bar" into single *StringNode.
1 participant