Add ability to choose a different formatter#646
Conversation
|
Thanks for the test. I am pushing a version in a few that passes all specs in my Ruby 3.4, and we can then see if it passes in other Rubys. I can't tell anything from the ArgumentError you observed, other than the fact that this is an internal Ruby error, and not a bashly / shfmt error - so perhaps differences between our ruby versions - which one you use? |
|
|
Ok - so specs pass now (although there is no spec for the new formatter yet). I am not sure why you get that Can you run Another debugging option, is to open the |
DannyBen
changed the title
internal, none, shfmt)
|
The error seems to be related to the fact that Open3#capture3 expects to receive a string as the first argument, and you're passing an array of strings Open3.capture3 %w[shfmt --case-indent --indent 2], stdin_data: scriptThe documentation also mentions that you can use several strings in args, like this: Open3.capture3('echo', 'C #')
# => ["C #\n", "", #<Process::Status: pid 2282368 exit 0>]
Open3.capture3('echo', 'hello', 'world')
# => ["hello world\n", "", #<Process::Status: pid 2282372 exit 0>]But I think that as you're passing an array of strings followed by an implicit hash DEBUG=1 outputBy the way, I've tested with |
|
Excellent. I am pushing a version in a few with support for custom command, stay tuned. |
|
I am ready to merge. |
|
I'll be available for careful testing over this weekend. |
Cool. I will also be available for fixes. I am marking this PR as pending your approval. |
|
@pcrockett since it was you who nudged us in this direction, if you have time to review, I am sure it will be beneficial. |
DannyBen
changed the title
internal, none, shfmt)
|
@DannyBen it might take me a few days to get around to thoroughly reviewing or testing. but i am interested in it. in any case i won't be offended if you merge this before i get to it. |
|
in any case, the feature as described by the docs sounds great. 👍 |
No pressure. I will wait for several days at least as I prefer merging after your input than having to change it later. |
Co-authored-by: meleu <meleu@users.noreply.github.com>
|
@EmilyGraceSeville7cf since I see you are around - if you have comments on my settings schema update, let me know. |
|
I've manually tested the scenarios specified in Other scenarios that are not in the spec and I've tested:
|
meleu
left a comment
meleu
left a comment
There was a problem hiding this comment.
I'm happy with the results of this PR.
Nice job! 👍
pcrockett
left a comment
pcrockett
left a comment
There was a problem hiding this comment.
I played with it. I like it. Looks good.
I briefly had some doubts about the "custom command" mode. The only purpose it would serve is to allow users to specify their formatting commands in a Bashly settings file rather than a Makefile or GH Actions workflow.
But I think that can be kind of nice -- you control your script output from one YAML file. So it's worth keeping, especially since it looks like it'll be fairly straightforward to maintain.
3 participants
cc #644, #645
This PR comes to replace #645, and solve the consecutive heredoc newlines using a different approach.
formatter: internal.nonewill avoid compacting newlines completely.externalwill format it usingshfmt --case-indent --indent 2.shfmt --minify) will assume this is the command to run, and run it as a formatter. The command will be sent the pre-formatted script through stdin, and is expected to print the formatted script to stdout.The previous
String#lintextension method was split into two:String#remove_private_commentswhich removes the## commentsfrom the scriptScript::FormatterclassTODO