Improve generated shell completion scripts

[Ordoviz]

I added some double quotes around variables to prevent globbing and word splitting and turned the completions array into a string since to simplify the code. There are two user-visible changes:

1. Completions for filenames with spaces will no longer be split (but filenames with newlines still will be)
2. Added missing dollar sign in front of a Fish variable name

[adaszko]

Hi and thank you for your contribution!

Completions for filenames with spaces will no longer be split (but filenames with newlines still will be)

Do you have some example use case that isn’t working for you? I just checked and bash completions handle filenames with spaces well. Could you write an e2e test (see the e2e subdirectory) for your use case?

[Ordoviz]

Do you have some example use case that isn’t working for you? I just checked and bash completions handle filenames with spaces well. Could you write an e2e test (see the e2e subdirectory) for your use case?

$ complgen compile examples/predefined.usage --bash-script /tmp/a.bash                                                                                                                                      3s
$ mkdir /tmp/dir && cd /tmp/dir
$ mkdir 'dir with spaces'
$ source /tmp/a.bash
$ cmd<TAB>
dir     spaces  with    

[adaszko]

Do you have some example use case that isn’t working for you? I just checked and bash completions handle filenames with spaces well. Could you write an e2e test (see the e2e subdirectory) for your use case?

$ complgen compile examples/predefined.usage --bash-script /tmp/a.bash                                                                                                                                      3s
$ mkdir /tmp/dir && cd /tmp/dir
$ mkdir 'dir with spaces'
$ source /tmp/a.bash
$ cmd<TAB>
dir     spaces  with    

I’ve reproduced this as a e2e test. Would you mind putting that in e2e/test_bash.py and replicating the same test case for other shells too? You run tests with the ./run-e2e-tests.bash script. I want to preserve your attribution.

def test_completes_dir_with_spaces(complgen_binary_path: Path):
    with completion_script_path(complgen_binary_path, '''cmd <PATH>;''') as completions_file_path:
        with tempfile.TemporaryDirectory() as dir:
            with set_working_dir(Path(dir)):
                os.mkdir('dir with spaces')
                completions = get_sorted_completions(completions_file_path, '''COMP_WORDS=(cmd); COMP_CWORD=1; _cmd; printf '%s\n' "${COMPREPLY[@]}"''')
                assert completions == sorted(['dir with spaces'])

[adaszko]

I’ve reproduced this as a e2e test. Would you mind putting that in e2e/test_bash.py and replicating the same test case for other shells too? You run tests with the ./run-e2e-tests.bash script. I want to preserve your attribution.

Just a note: I you feel like fixing other shells besides bash is too big of an undertaking, I will gladly accept patches just for bash. I didn’t mean to overburden you :)

[Ordoviz]

This is almost ready for merging now. The Zsh completion for "dir with spaces" is just missing a trailing slash as in "bar/". I will leave that for you to fix or ignore :smile:

[adaszko]

OK, awesome. If you could apply that one suggestion I made to keep all tests passing and squash the two commits that cancel each other out (543dcbbb99568c1dc35a6adcbfe26bfcf64b9997 and 8620ab0715ae0d624f05daefaac97d73d60355ab) and this PR is good to go!

[Ordoviz]

I replaced "dir with spaces/" with "file with spaces" in Zsh. This is ready for merging now.

[adaszko]

I'll look into merging this ASAP (currently mostly AFK)

[adaszko]

Great improvements. Many thanks!

[adaszko]

It would even make sense to incorporate shellcheck into the testing pipeline