-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
wip: syntax: flush heredocs before right parens #924
wip: syntax: flush heredocs before right parens #924
Conversation
e.g. when using heredocs in command substitutions, process substitutions, and subshells
}, | ||
{ | ||
"diff -y <(cat <<EOF\n1\n2\n3\nEOF\n) <(cat <<EOF\n1\n4\n3\nEOF\n)", | ||
"diff -y <(cat \\\n<<EOF\n1\n2\n3\nEOF\n) <(\ncat <<EOF\n1\n4\n3\nEOF\n)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I realized while writing these examples is minified heredocs might not necessarily be minified! For example the following is 21 bytes:
a=$(cat <<EOF
EOF
)
And produces the following 22 bytes:
a=$(cat \
<<EOF
EOF
)
It's because of the cat \
, but digging through the code I couldn't immediately see what causing that, or how to address it without breaking too much else. In the meantime, I figure valid shell is a lot better than invalid shell though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff
example in the test is very funky, because the minified code has the first cat
continuing its <<EOF
onto the next line, but the next cat
in the second proc subst is left alone as cat <<EOF
.
Oh no, sorry this is embarrassing... I foolishly was only running This change as it is breaks a few other wacky cases, like:
I did some more digging through the code to see if I can satisfy both (the existing test case of a subshell after the beginning heredoc word, and the new one I added where the heredoc is within a subshell), but I think it would involve a new printer field to keep track of if we're in a heredoc or not. |
Thanks for the PR! I did see it, I just haven't had the time to sit down and debug what's going on :) |
Finally figured it out, it was a bit subtle :) I also noticed that the escaped newlines right before |
Oh - thanks! Sorry this got lost in the inbox. Time sure does fly. Thanks so much! Really appreciate you and this project! 🥳 |
Fixes #923 😄