-
-
Notifications
You must be signed in to change notification settings - Fork 93
Fix consecutive newlines in heredoc not being preserved #645
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
Conversation
I would love some input and comments from heavy bashly users or collaborators, to help decide if this PR should be merged or not.
|
Talking strictly with a bashly user point of view: I see what is reported in #644 as an actual bug. Therefore something to be solved. I would be very disappointed if any behavior coded in a file inside
When you mention "overhead", I'm not sure if you're talking about code maintenance cost or increased build time. If it's about the build time, I would happily trade a few hundred milliseconds in script generation time for not being surprised with a situation like the reported one. If the solution in this MR is not ideal/satisfactory. We can hold it and try to find a better one, but I still think #644 is indeed a bug (maybe more critical than the indentation one, as it changes the behavior). |
Thanks @meleu,
This is already not precisely replicated, as it is both indented and linted (mainly duplicate newlines removal). Another possible solution instead of trying to fix the newline linter, is to try and remove the need for it.
Overhead mainly in code maintenance and complexity. This solution adds another - separate - class for handling yet another heredoc edge case.
Yes. It is far from ideal, and intuitively feels like a bloat to me. Perhaps a deeper look into how to eliminate the side effect, which is mainly caused by the ERB templates. |
Thanks to the expressiveness of Ruby and your talent to use it beautifully, I think the
I'm a bit confused here... You mean the possibility to use ERB in config is the root cause of this complexity? |
No. Bashly uses GTX templates, which is just a simple wrapper around ERB, to provide a different syntax more suitable for bashly's templates (Ruby code first instead of template output first). When concatenating several ERB (or GTX) templates together, sequences of newlines form in a way that is not so easy to eliminate. This is also happening in To see this behavior, one can eliminate the linting function and generate a bashly script. You will see many newlines that are mostly impossible to remove by updating the templates. This is the root cause. |
Here is a minimal demonstration of the problem that the linter is fixing. ERBrequire 'erb'
insert = <<~TEMPLATE
sub template
TEMPLATE
subtemplate = ERB.new(insert).result(binding)
template = <<~TEMPLATE
hello
<%= subtemplate %>
<%= another_template if false %>
<%= another_template if false %>
world
TEMPLATE
puts ERB.new(template).result(binding) Output
GTX - this pattern appears a lot in bashly templatesrequire 'gtx'
subtemplate = GTX.new("> sub template").parse
template = <<~GTX
> hello
= subtemplate
= subtemplate if false
= subtemplate if false
= subtemplate if false
> world
GTX
gtx = GTX.new template
p gtx.parse binding Output
|
Thanks for the clarification, now I understand the issue. The first question that comes to my mind is: do these extra lines affect the behavior of the final script, or is it a code aesthetics concern? (Note: I'm not implying that code aesthetics isn't important, just pondering.)
Can you give me instructions to achieve this (generate a script with no linting)? The fictitious scenario I'm wondering here is: if shfmt was a Ruby gem, would it be ok to remove all this extra-lines cleanup logic and just format the generated script at the end? |
Not at all, similarly to indentation.
Sure. Assuming you have Ruby 3.2 or higher installed: # clone the repo
git clone https://github.com/DannyBen/bashly.git
cd bashly
# update the file `lib/bashly/extensions/string.rb
# look for the `lint` function and replace it with this:
def lint
self
end
# now you can work inside a folder named "dev" (gitignored)
# and work with bashly almost normally by prefixing it with `bundle exec`
mkdir dev
cd dev
bundle exec bashly init
bundle exec bashly generate
This is a great question, even if it was NOT a Ruby Gem. Some linters in some languages do that exactly, even more rigorously - they also add newlines when it is appropriate - even Rubocop in Ruby, and some if not all python linters. One example that comes to mind, in Rubocop, after a # this is not linted
somefunc() {
echo "one";
echo "two";
} Bottom line - linting implies proper (not too many, not too little) empty lines. |
I think I like where this thought is going. As far as I'm concerned, it doesn't even need to be a Ruby gem. You could just make this an optional dependency. Have something in your docs to the effect of, "If you want a pretty / shfmt-compliant script, make sure you have I have encountered other tools which try to massage code for me, but try to do so with regular expressions or other hackery, without using a proper abstract syntax tree. This way of doing things is doomed to unending edge case bugs. These tools are infuriating. If we aren't prepared to actually parse the script into an AST before manipulating it, I think running |
Hmm. This is interesting, I was unsure that shfmt lints newlines. While I like the idea of using purpose specific tools, it does feel like we let the tail wag the dog here. |
Here is a direction I would like to consider:
Thoughts? |
I think that sounds pretty good. I'd change the term "linter" to "formatter." Linter implies we're going to return errors, when script formatting is what we're actually doing.
Aug 1, 2025 10:24:14 Danny Ben Shitrit ***@***.***>:
…
[Image]*DannyBen* left a comment (bashly-framework/bashly#645)[#645 (comment)]
Here is a direction I would like to consider:
1. Revert this sub-par PR back, and go back to simple consecutive newline squasher.
2. Add a bashly setting called *lint* or *linter*:
linter: internal # default behavior
linter: none / false / no # no linting at all (other than removing the private comments
linter: shfmt # run an internally defined shfmt command
linter: some_other_command %temp_script_path% --options # maybe even support this
Thoughts?
—
Reply to this email directly, view it on GitHub[#645 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAIQCEMMVO6TX6WYXOV4BA33LMP2ZAVCNFSM6AAAAACCTNTZOGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNBTGY4TQOBWHA].
You are receiving this because you were mentioned.
[Tracking image][https://github.com/notifications/beacon/AAIQCELLAGVPMZ6FVRMKH2L3LMP2ZA5CNFSM6AAAAACCTNTZOGWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTV3MEE3I.gif]
|
Closing this, as it was fixed in a different way in #646. |
cc #644
The referenced issue discovered an edge case with heredocs, where consecutive newlines are being squashed to a single newline.
The solution was to expand the linting operation, from a simple single line that replaced multiple newlines with one, to a full blown
LintHelper
class, similar to theIndentationHelper
class (for heredoc-aware indentation).Tasks