Always strip trailing spaces in TabularizeStrings

[jamessan]

Currently, trailing spaces are only stripped from the first element in a line if it contains non-whitespace characters, however this causes padding creep.

Given a block like

    foo   => bar
    beezlebub
          => debil
    john  => doe

:GTab /=>/ will move the => column to the right by one instead of to the left by one.  Similarly, using :Tab /=>/ twice will first align the column as expected, but then shift the column one to the right every time you repeat the command.

This is because the leading whitespace on the => debil line is considered when calculating the max line length.

Stripping the trailing whitespace (which is all of the whitespace in this case) ensures the lines are all aligned, only taking into account relevant content.

[godlygeek]

Hm. The code has this comment:

" Strip spaces
"   - Only from non-delimiters; spaces in delimiters must have been matched
"     intentionally
"   - Don't strip leading spaces from the first element; we like indenting.

I'm sure the reason I wrote the if line[0] !~ '^\s*$' check was that, if the element is all whitespace then that is leading as well as trailing whitespace. So, the only thing this code accomplishes is preventing the indent of a line from being reduced...

int some_c_function()
{
    // cond1 cond2 cond1&cond2
    // 0 0 0
    // 0 1 0
    // 1 0 0
    // 1 1 1
}

Doing :3,7Tab/\S\+ currently gives

int some_c_function()
{
    //  cond1  cond2  cond1&cond2
    //  0      0      0
    //  0      1      0
    //  1      0      0
    //  1      1      1
}

But with this patch applied it would give

int some_c_function()
{
//  cond1  cond2  cond1&cond2
//  0      0      0
//  0      1      0
//  1      0      0
//  1      1      1
}

I don't know how I feel about that. I'm inclined to think that this is a feature, not a bug - Tabular won't remove your indentation, no matter what args you give it. If you don't like that, you can always use :left before your call to :Tabularize.

Thoughts on the tradeoffs, jamessan?

[jamessan]

I wonder how common either case is. I would argue that in your example the pattern being fed to :Tabularize isn't correct, but I concede that it is the obvious one and coming up with a Correct(tm) pattern would be more difficult than it's worth.

That being said, with my proposed changes the behavior in both scenarios is at least stable. Your snippet gets thrown to the left and stays there. My snippet gets properly aligned. Tabular's current code aligns your example and shifts my example to the right every time you call :Tabularize. Fixing your snippet is a simple matter of a block insert. It may not be that simple for mine since the block selection that would be need to remove the right shift may go through actual text that can't be deleted.

That's my take. Do what you think makes the most sense, as I doubt I'll run into this very often since I'm not generally a fan of alignment. I just use your plugin when I need to do it and found this behavior very confusing. :)

[jamessan]

but I concede that it is the obvious one and coming up with a Correct(tm) pattern would be more difficult than it's worth

I was just revisiting this PR based on a discussion in #vim and noticed that it's not nearly as difficult as I originally thought -- :3,7Tab/\w\S*

[godlygeek]

I'll grant that /\w\S*/ works, but swapping the delimiter for the delimited is quite a non-obvious trick...

Well, anyway - what was the recent conversation in #vim about? Did someone else bump into this behavior and find it surprising? If this is a behavior that people are bumping against regularly, then I'll concede that the behavior needs changing - but I still think that preserving the indent is useful, too.

So, then - maybe the best fix is to find the common leading whitespace, strip the common indent from all lines, align the lines (without treating the first field as special), and then add the common whitespace back to all lines? I think that a patch that does that would fix the creeping indent in a mostly backwards compatible way...

[jamessan]

There was just a discussion about alignment plugins and whether the functionality of easy-align is worth its complexity or if the common use cases are covered by the simplicity of Tabular. I brought up that Tabular had been enough for me save the one edge case I ran into here.

I'll take a stab at implementing your suggestion "soon".

[godlygeek]

Something like dcd43cb (the new trim_1st_field branch) might be a good start. I haven't tested this fully, but it seems to at least be a step in the right direction. In addition needing more testing, I still need to figure out whether or not the lead_blank logic should stay...

[jamessan]

That looks good. I made some adjustments in jamessan/tabular@a2ad74f which now makes it satisfy both of our scenarios.

[godlygeek]

I missed that blank lines need to not break an indent streak - good catch.

I'm not sure about the fix to have LongestCommonIndent match against the delimiter, though - it seems like the wrong approach to me. It also made me spot an issue that even my approach had: because indent was stripped before field splitting was done, it'd be possible for the indent stripping to change a line from matching to not matching the delimiter.

My current line of thought is that the indent handling should happen after the SplitDelim call is made, instead - your latest patch moves the stripping to after SplitDelim, but I think we also want the LongestCommonIndent calculation to be moved as well.

I'm going to play with that and try to get you a new patch to try out...

[godlygeek]

Easier than I thought - try out ca692a7 and see how it works for you. I still want to do some more testing, but this definitely seems cleaner, more consistent, and more correct than my first attempt.

[godlygeek]

Gah, actually - that doesn't work right for the truth table example I gave above. Hrm.

[godlygeek]

Third time's a charm - try out a6ef92c if you get a chance.

[jamessan]

👍

[godlygeek]

I've merged my proposed fix as cdac1a6