search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.1k stars 336 forks source link

v4: syntax: keep alignment of command-continuing backslashes #498

Open Varriount opened 4 years ago

Varriount commented 4 years ago

Currently shfmt strips trailing spaces between the last word of line and a backslash:

a_command   \
  with      \
  many      \
  options   \
  and       \
  arguments

becomes

a_command \
  with \
  many \
  options \
  and \
  arguments

It would be nice if shfmt could either keep this spacing or ideally automatically maintain the alignment.

mvdan commented 4 years ago

This issue is extremely similar to https://github.com/mvdan/sh/issues/390; please take a look.

mvdan commented 4 years ago

Friendly ping @Varriount.

Also CC @dcsobral, who didn't reply on the original thread.

The simplest change we could do is enforce these to be aligned all the time, like we do with comments.

The alternative would be to have -kp keep their padding, and not enforce any alignment ourselves. This would be harder, as it would require us to add the \ characters to the syntax tree somewhere, with position information.

The third alternative is to keep things as is and do nothing.

Varriount commented 4 years ago

Personally, I would prefer the first option, however I understand that not everyone may like that style.

Wouldn't both 1 and 2 require adding position information?

mvdan commented 4 years ago

Option 2 definitely needs position information, as it needs to retain it from the original source code.

I think option 1 does not need it, because the original column number of each \ is irrelevant. We only care about how many \ are there in a sequence of lines, and then we align them all in a post-processing step, just like we do with comments. Similarly, a comment's column position doesn't really affect how a script is formatted, it's only there for other use cases.

oliv3r commented 4 years ago

I think both styles would need to be allowed though, and shfmt should be smart enough to figure out what you indended. e.g. if there's just one space, it's likely that's what was desired, if there's more then one, the intention probably is to align 'to the right'. Problems arise of course when not enough information is available, but that is also easily resolved by the user fixing it once.

mvdan commented 4 years ago

@oliv3r detecting what the user wanted to do is definitely not something I think we should do for something as brittle as vertical alignment. The heuristic would be complex to write and maintain, and probably annoy users.

We do have a couple of heuristics in place, like "did the user write a function in a single line", but they are very few and extremely simple in comparison.

If we think this alignment is something we should enforce, like comments, we should just do it all the time. I slightly lean towards this option.

If we think it would be too annoying, then I think we should go with -kp, and let the user pick whatever alignment or whitespace padding they want.

mvdan commented 4 years ago

To clarify my point above - option 1, to enforce this alignment, is something we might have to leave for a future v4 since it's a breaking change. Otherwise, if done in shfmt v3.2.0, the tool would disagree with previous versions of the tool like v3.1.0. That's one short term disadvantage of this option, though I like it better in the long term.

dcsobral commented 4 years ago

I prefer option number 2, though number 1 is fine as well.

-- Daniel C. Sobral

On Tue, Feb 25, 2020 at 1:28 PM Daniel Martí notifications@github.com wrote:

To clarify my point above - option 1, to enforce this alignment, is something we might have to leave for a future v4 since it's a breaking change. Otherwise, if done in shfmt v3.2.0, the tool would disagree with previous versions of the tool like v3.1.0. That's one short term disadvantage of this option, though I like it better in the long term.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvdan/sh/issues/498?email_source=notifications&email_token=AABCOF3ZJ3Z57LAU7YJXSULREV5NVA5CNFSM4KSTAIHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM5L7OI#issuecomment-591052729, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABCOF2CCKGZ3OFPRBM7UFTREV5NVANCNFSM4KSTAIHA .

mvdan commented 4 years ago

The alternative would be to have -kp keep their padding, and not enforce any alignment ourselves. This would be harder, as it would require us to add the \ characters to the syntax tree somewhere, with position information.

I've thought some more about this, and we can't do this in v3 either, because it would require a significant modification of the syntax tree nodes, to the point that it would be a breaking change and likely break plenty of programs using the syntax package (including shfmt itself).

So, whichever way we choose, I think this needs to wait until a future v4. I'm fairly certain that will happen, but I can't say when, because I don't want to break users too often (and there's plenty of work to be done in v3 without breaking backwards compatibility.

Between the two options, I still lean towards the first - just aligning those backslashes - because we align comments as well, and it feels like a simpler and more consistent solution.

mvdan commented 3 years ago

After much thought over a number of years, I'm considering removing this feature in the next major release. See https://github.com/mvdan/sh/issues/658, and please try to keep an open mind when considering the benefits and drawbacks of the flag. Thanks!

It's also worth noting that this issue could still get fixed by always aligning "line continuation" backslashes, as mentioned in earlier comments.

maxsu commented 2 years ago

The simplest change we could do is enforce these to be aligned all the time, like we do with comments.

shfmt's comment alignment feature was a nice surprise, and really boosted my code readability. It would be nice to have the same for continuations!

I might try to implement it! Could you point me to the code that post-processes the comment lines? I hope I might be able to adapt that code for continuations.

So far:

Now I suspect the post-processing might be entangled with a few other stories, so that it might not be so easy for me to add the continuation feature. But I'm not giving up hope just yet! Can you point me in the right direction?

mvdan commented 2 years ago

@maxsu for sure! what implements this is actually https://pkg.go.dev/text/tabwriter, so you want to grep for tabwriter in printer.go. Any tab characters which aren't surrounded by tabwriter.Escape then get used for vertical alignment, for example the following is the tab character used for comments: https://github.com/mvdan/sh/blob/07eccc224ba84c5b21b308654266b4098e5c28c9/syntax/printer.go#L558

maxsu commented 2 years ago

Thanks for the comment!

Sounds like my plan here is:

Does this sound like a reasonable plan of attack?

mvdan commented 2 years ago

That sounds like a good plan. I don't think v4 will need a toggle, but we should probably add one in v3, for the sake of not breaking backwards compatibility.