v4: remove syntax.KeepPadding and shfmt -kp

[mvdan]

The point of this feature was to make using shfmt more reasonable for large existing codebases, which might be using custom formatting with regards to alignment or padding. For example:

func one   two   three
func four  five  six
func seven eight nine

The intent was good, but I think the idea is ultimately not a great one. I've spent a significant amount of code and time towards this feature, and it's still riddled with bugs and edge cases that misbehave (1, 2). Someone also proposed redesigning the entire feature, which might count as a breaking change in itself.

I think it's time to reconsider whether this flag is a good idea at all. I already outlined the main advantage above. Now to list the disadvantages:

• Extra cost in size, complexity, maintenance, and bugs
• More formatting flags tend to mean less consistency in format and a worse UX for the average user
• Conflicts with core features such as consistent spacing between tokens and vertical comment alignment
• Not clear how the feature should work; should it count column offsets, or relative offsets? Should it do escaped newlines too?

All in all, this is a common source of bugs and maintenance pain, and honestly I have never used the feature myself. A good portion of the -kp users stopped using the flag after they realised the downsides, and overall they got better formatting too; here's an example.

So I propose to entirely remove this feature in v4. Anyone who wants to keep using this feature, I'd ask you to give the formatter an honest try without the flag. If any programs get formatted in a clearly worse way, I'd like to hear about those - perhaps we can fix those cases independently of -kp.

I'll point the existing KeepPadding threads towards this one for input. Thanks for reading!

[mvdan]

As for breaking existing users, it's clear this would be a breaking change, hence why it cannot happen in the current v3. If a v4 removes this feature, attempting to use the command-line flag or EditorConfig option would result in an error.

It's also worth noting we could always bring the feature back in the future in some other form, if we so wished.

[cornfeedhobo]

I support this move, given the experience we're going through with bash-it. If I had more time, I'd maybe take a look at re-writing this feature, but I just don't have the time I wish I did. Thanks for the hard work.

[maximbaz]

As already mentioned in another thread, I support the reasons for the move, and I disabled -kp in my daily usage to find some examples where the experience becomes noticeably worse - and to be honest I found only one, but I think it is actually the one that made me start using -kp in the first place, so I would like to share it.

I tend to work with a lot of PKGBUILD files for Arch Linux, they are basically shell scripts that follow a specific format, and a specific styling convention. Take a look at this one, for example: PKGBUILD

If you try to run shfmt on it, you will notice that all the blocks in the top of the file will be moved to the left. Unfortunately, the current style is de-facto used by virtually all people who write PKGBUILD files, so what shfmt does is undesirable, it adds inconsistency. Furthermore, the official Arch Linux tools to help writing PKGBUILD files also enforce this style, for example run updpkgsums (part of pacman-contrib package) and you will see that it will undo whatever shfmt did for the sha256sums block.

What this means for me is that I basically can no longer use shfmt when editing PKGBUILD files 😞

[mvdan]

Thank you, that's helpful input.

If you try to run shfmt on it, you will notice that all the blocks in the top of the file will be moved to the left

So it seems like the main incompatibility is the indent-alignment of arrays. The good news is that such a problem can be tackled with a much simpler solution than -kp. For example, one can imagine that, if space indentation is used and arrays begin with an element on the same line, then the elements on following lines could be vertically aligned. This is something we already do for comments, for instance.

The canonical format, when indenting with -i=4, could be:

# The first element is on the same line; align elements vertically.
aligned=(one
         two
         three)

# The first element is on a separate line and indented,
# so we don't need vertical alignment.
indented=(
    one
    two
    three
)

That's something we could certainly try, if it sounds like a good idea. It's unclear if it would be a breaking change in v3, but we could always try implementing it and see how many users it upsets. I think, overall, it should result in better style. And if anyone does not like the alignment, they can just add a newline before the first element to get the good old indentation.

This new feature would only kick in for space indentation, not tabs, because mixing tabs and spaces is deliberately not supported.

[maximbaz]

I like this proposal, let's try!

[mvdan]

Alright, I've split that into its own issue, since I want to leave this issue about removing -kp in v4.

[maximbaz]

Hello again 🙂

Was just writing a small script and noticed another example where I think the formatting is arguably worse without -kp:

case "$app" in
    org.qutebrowser.qutebrowser)
        case $1 in
            swipe-3-right) wtype -k escape -s 100 -M shift L -m shift ;;
            swipe-3-left)  wtype -k escape -s 100 -M shift H -m shift ;;
            swipe-3-up)    wtype -k escape -s 100 -M shift K -m shift ;;
            swipe-3-down)  wtype -k escape -s 100 -M shift J -m shift ;;
            swipe-4-up)    wtype -k escape -s 100 u ;;
            swipe-4-down)  wtype -k escape -s 100 d ;;
        esac
        ;;

Compare this to:

case "$app" in
    org.qutebrowser.qutebrowser)
        case $1 in
            swipe-3-right) wtype -k escape -s 100 -M shift L -m shift ;;
            swipe-3-left) wtype -k escape -s 100 -M shift H -m shift ;;
            swipe-3-up) wtype -k escape -s 100 -M shift K -m shift ;;
            swipe-3-down) wtype -k escape -s 100 -M shift J -m shift ;;
            swipe-4-up) wtype -k escape -s 100 u ;;
            swipe-4-down) wtype -k escape -s 100 d ;;
        esac
        ;;

In my mind the latter is much harder to evaluate in your head, when you are just reading the file, or to compare the behavior between multiple case statements...

What do you think about making a case to handle single-line case statements be aligned?

[mvdan]

Thinking outloud, what about this format instead?

case "$app" in
    org.qutebrowser.qutebrowser)
        case $1 in
            swipe-3-right)
                wtype -k escape -s 100 -M shift L -m shift ;;
            swipe-3-left)
                wtype -k escape -s 100 -M shift H -m shift ;;
            swipe-3-up)
                wtype -k escape -s 100 -M shift K -m shift ;;
            swipe-3-down)
                wtype -k escape -s 100 -M shift J -m shift ;;
            swipe-4-up)
                wtype -k escape -s 100 u ;;
            swipe-4-down)
                wtype -k escape -s 100 d ;;
        esac
        ;;

This might end up being too verbose since shfmt will likely place the ;; statements on separate lines, but we could change that for this scenario.

I don't have anything against vertical alignment per se, but I'm initially reluctant to add more forms of it. This case you suggest seems reasonable, though.

[maximbaz]

It's also a valid formatting, but choosing between the two, in this particular example where each case consists of one line only, I would strongly prefer the formatting in my example, it's much more condensed and in my mind has better readability, especially if it's valuable to compare different case branches at a glance.

[mvdan]

Fair enough. Can you open a separate issue about it, like we did with arrays?

[mvdan]

v4 will definitely drop the "keep padding" flag. I'm closing all existing bug reports in favor of this issue, to track removing the feature. Thanks everyone for the input!