fish: Abbreviations with empty expansion produce invalid fish syntax

[anmayithap]

Problem

The mkAbbrStr function unconditionally concatenates "${abbr.expansion}" at the end of the generated command:

mkAbbrStr = abbr:
  (foldl' (
    acc: elem: acc + " " + (mkAbbrArg elem abbr)
  ) "abbr --add ${abbr.word} ${mkCursorArg abbr}" abbrArgs)
  + " "
  + "\"${abbr.expansion}\"";

This causes two related problems:

1. Empty string bug: When expansion = "", fish receives a trailing empty argument:

abbr --add dot-dot --regex "^\.\.+\$" ""

2. --function incompatibility: Fish abbr --function does not require an expansion at all. However, because expansion is declared as types.str with no default, users must pass expansion = "" to satisfy the module, which then generates the invalid trailing "".

Reproduction:

{
  abbreviations.myabbr = {
    word = "myabbr";
    function = "my_function";
    expansion = "";  # forced to satisfy types.str, but meaningless to fish
  };
}

[BirdeeHub]

Oh! So it does.

This is somewhat of a design issue with this option...

@ormoyo any advice?

I am thinking of a few options that jump off the top of my head

Problem is, some of them are breaking changes.

The most obvious but most breaking would be, make it a submodule type instead of a spec type

This might be considered breaking enough that we should come up with a new name for it and deprecate the old as gracefully as possible.

Outside of that, we could leave the type the same but allow null for "not provided", or ignore empty values. These would be much simpler changes, but require people to do

{
  abbreviations.myabbr = {
    word = "myabbr";
    function = "my_function";
    expansion = null; # required
  };
}

I am of course also open to any other decent ideas I haven't listed yet.

[ormoyo]

It seems I should've done some more proper testing, I haven't actually tested the options much except making them valid syntax (except cases like this I guess).

In most cases (at least imo) abbreviations replace a word with an expansion, which is great for the spec normalization.

I think the easiest thing would be allowing null for expansions, it's a 2 lines change and we avoid breaking changes.
It will be pointed in the docs and then it's done.

[BirdeeHub]

It would work and be a minimal change yes.

That being said, I don't use fish at all whatsoever.

So I can't have any input as to which is more common.

But, given there are also aliases and scripts, and it is specifically called abbreviations, that does make some sense to me.

@anmayithap do you have any input on this? More input is generally a good thing.

Personally I feel that is probably the best path forward given it would be a breaking change to change the type.

[anmayithap]

@BirdeeHub in my view, it would be best not to change the types, since they generally conform to the specification. However, the issue lies with the mkAbbrStr function: it should omit expansion when the --function argument is provided, as the abbr specification indicates:

abbr --add NAME [--position command | anywhere] [-r | --regex PATTERN] [-c | --command COMMAND]
                [--set-cursor[=MARKER]] ([-f | --function FUNCTION] | EXPANSION)

[ormoyo]

Oh, that's right!

If only in the case where --function is specified the expansion needs to be omitted then changing the function like @anmayithap said will solve it:

mkAbbrStr = abbr:
  (foldl' (
    acc: elem: acc + " " + (mkAbbrArg elem abbr)
  ) "abbr --add ${abbr.word} ${mkCursorArg abbr}" abbrArgs)
  + optionalString (abbr.function == null) " \"${abbr.expansion}\"";