:trim to remove trailing whitespace

[kirawi]

Supersedes #3674 and resolves #1481

:trim will remove both trailing empty (lines consisting of only whitespace characters) and trailing spaces on lines. The superseded PR did this as well, but the code became harder to read/reason about (for me) while trying to keep everything in a single transaction, as it allowed you to choose to trim either spaces, lines, or both. This implementation will always trim both.

Edit: I just noticed that it would actually be trivial to reintroduce those options because I ended up implementing this differently than before. It would just be adding lines (would replace var trailing) and spaces as arguments and conditionals to the existing if statements. I won't be doing that though.

[Joe-Zer0]

I would very much like a config flag to run this command on write. Is that something you would consider adding?

[kirawi]

I'll think about it, but at the moment I chose not to so the PR would be simple.

[pascalkuthe]

That should be a seperate PR and likely based on #8021. Maybe it doesn't even need a special config flag and instead we can allow users to specify a list of commands to run pre-write

[Joe-Zer0]

Sounds good. Thanks for the PR by the way. It's a feature I'm really looking forward to. I was actually attempting to do the same thing based on the code from your last PR. But it was slow going as I was trying to learn Rust along the way :)

[lelgenio]

What about removing leading lines? When working with bigger expressions I tend to add a few empty lines around it to better focus on it, :trim could be useful to clean-up in such cases if it was able to remove leading lines.

Before:

Animals::find().filter(AnimalType::Horse).for_each(|horse|{
    horse.feed(
// ⬅️ selection-start

    
      CarrotBuilder::new()
        .color(Color::Orange)
        .size(Size::Big)
        .taste(Taste::Good)
        .build()


// ⬅️ selection-end
    );
});

After :trim:

Animals::find().filter(AnimalType::Horse).for_each(|horse|{
    horse.feed(
      CarrotBuilder::new()
        .color(Color::Orange)
        .size(Size::Big)
        .taste(Taste::Good)
        .build()
    );
});

To me it also makes sense to remove leading spaces:

Before:

horse.feed(⬇️      Carrot::new()  ⬇️);

After :trim:

horse.feed(Carrot::new());

[kirawi]

That's more or less the job of a formatter. The cases you describe are going to be affected by how the language is defined. For instance, how can you determine whether to trim leading spaces or not when the user might want to keep leading spaces in strings, e.g. " Hello, world!" Or in the case of leading empty lines, whether it fits the coding style? At the point you start going down a rabbit hole of exceptions and cases until you essentially end up with a formatter.

If you really want to do the cases you're talking about without :fmt, you can split your selection to empty lines and leading spaces and then d. :trim is not necessary in that case.

[leandrobbraga]

That's more or less the job of a formatter. The cases you describe are going to be affected by how the language is defined. For instance, how can you determine whether to trim leading spaces or not when the user might want to keep leading spaces in strings, e.g. " Hello, world!" Or in the case of leading empty lines, whether it fits the coding style? At the point you start going down a rabbit hole of exceptions and cases until you essentially end up with a formatter.

If you really want to do the cases you're talking about without :fmt, you can split your selection to empty lines and leading spaces and then d. :trim is not necessary in that case.

Since we want to remove only trailling whitespaces, shouldn't we call this :rtrim: or other name algother? It would be less surprising since most languages I know contains a triplet of funtions: trim, ltrim and rtrim. The trim usually strips from both directions.

See:
rust: https://doc.rust-lang.org/std/primitive.str.html#method.trim
javascript: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trim
python: https://docs.python.org/3/library/stdtypes.html (called strip)

[the-mikedavis]

I don't forsee us adding a trim-leading-whitespace feature so "rtrim"/"rstrip" seems unnecessary to me. If we want to be really clear let's call the command :trim-trailing-whitespace and alias it to :trim.

[gw]

Just wanted to drop by and say I would love this feature 🙏

[hovsater]

Would perhaps make sense to sync up with work done in #1777 given that EditorConfig have an option for trimming trailing whitespace as well.

[kirawi]

I've been running this PR for a few months now. While I don't use it very often, it hasn't caused any issues for me so far.

[kirawi]

I'm wondering if we want this or a way to select all trailing whitespace.

[tgross35]

I'm wondering if we want this or a way to select all trailing whitespace.

Deleting trailing whitespace seems significantly more common than needing to otherwise manipulate trailing whitespace - for anyone who does need to manipulate, I would think that just using \W+$ is suitable.

Could :trim be added and, if it turns out there is a valid usecase for selecting trailing whitespace, just turn :trim into an alias for select+delete?

[the-mikedavis]

Yeah I wonder if we really need a typable command for this given you can already select it with %s[ \t]+$ (could be bound to a macro with #4709) and then delete. Automatically trimming whitespace on save could be a nice addition though I think, similar to #8157

[kirawi]

Regex that approaches the behavior of this command is something like: (?-m)[ \t]+\n|\s+$. It's not perfectly accurate though since it will always remove the trailing line (EOF). I'm not sure there's a way around that.

[ArthaTi]

Is there any particular reason why this hasn't been merged yet?

[tgross35]

@the-mikedavis (or other maintainers) is this something that could be added to the next milestone? Even though there are workarounds (e.g. what is possible with #4709), having this out of the box is such a nice quality of life thing. Especially working with any file types that don't have autoformatters.

[the-mikedavis]

I wonder if instead of operating on each selection's range we should use Selection::line_ranges like we do for :reset-diff-change now. I don't think it ever makes sense to not operate on full lines.

[Rudxain]

I might have misunderstood the code or this review, but here are the pros & cons that come to mind right now:

• Trimming selections is much more versatile, but comes at the cost of hitting x before the cmd (if we want to trim the full line)
• Full-line-trim seems more common (in practice), so it should have a shorter key-sequence. But (mathematically) it's just 1 special case among an usize::MAX ("pseudo-Infinity") of potential selection-trims. Therefore, full-line-trim is rare (in theory)

Although, it seems fallacious to believe that FLT is common, simply because practically-all editors only support trimming all lines in a buffer, thereby forcing users to get used to this limitation

[the-mikedavis]

For a bit of a contrived example, say we have a file like:

····[········ffffffffffff···············
············ffffffffffff···············
············ffffffffffff·······]········

(with "·" being a space character and the selection being represented by the brackets)

I think :trim here should operate on all lines covered by the selection, resulting in

············ffffffffffff
············ffffffffffff
············ffffffffffff

where I think currently this PR would end up with

············ffffffffffff
············ffffffffffff
············ffffffffffff·······

I'm not sure it's ever useful to use the end of the selection range as the end of the area you're trimming

[Rudxain]

I agree. Now I see why it doesn't seem useful.

············ffffffffffff
············ffffffffffff
············ffffffffffff·······

In my mind, I thought it would be:

············ffffffffffff············
············ffffffffffff············
············ffffffffffff·······

Which makes less sense, I guess?

Then I thought "what if there were non-space code-points somewhere between the consecutive spaces?", then I answered my own question:

In practice, typically, users want 0, 1, or (rare) 2 spaces between non-space chars (let's ignore indentation, for simplicity). So, why would users need to specify how many spaces they want to delete? It'd be much simpler to:

• delete all spaces from all lines
• delete all spaces from all lines, except 1 space from the last line
• preserve some lines as-is

[Rudxain]

This looks like it could be a useful utility/helper somewhere else. Why was it defined locally? Is it to make the fn easier to call, as len: usize is more convenient than len: NonZero<usize>?