Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve fish completion script generation #738

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

rgoldberg
Copy link
Contributor

Improve fish completion script generation.

There are many issues with script generation. Some of them are documented in #679 & its comments.

This issue is for a first batch of fish completion script fixes.

Each commit should note the fix(es) that it contains.

Will rebase if main is updated (especially if #735 is merged).

Resolve #737

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@rgoldberg
Copy link
Contributor Author

@natecook1000 Rebased on the new main.

@rgoldberg rgoldberg mentioned this pull request Feb 16, 2025
4 tasks
Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Swift code looks great, but I'm seeing a few issues in the generated script. It looks like there are two syntax errors in the completion script for math, which I've annotated, and then at execution time I see unexpected output in a couple of situations:

  1. When trying to get completions for either of the arguments, I see this output when I press Tab after quantiles (I had to rename the executable to math2 to work around Fish's built-in math support):
~ $ math2 stats quantiles Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
  See 'math add --help' for more information.
test: Missing argument at index 3
-eq 2
      ^
in command substitution
Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
  See 'math add --help' for more information.
test: Missing argument at index 3
-eq 1
      ^
in command substitution
  1. The --custom completion worked, but printed an extra error message first:
~ $ math2 stats quantiles --file Desktop/add-preamble.txt --directory Desktop/Desktop/ --shell Abaris --custom Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
~ $ math2 stats quantiles --file Desktop/add-preamble.txt --directory Desktop/Desktop/ --shell Abaris --custom hel
hello  helicopter  heliotrope

Comment on lines 12 to 18
switch $POSITIONALS[1]
case 'average'
_swift_math_commands_and_positionals_helper '' 'kind= version h/help'
case 'stdev'
_swift_math_commands_and_positionals_helper '' 'version h/help'
case 'quantiles'
_swift_math_commands_and_positionals_helper '' 'file= directory= shell= custom= version h/help'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fish error: missing an end for this switch statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a fix for this.

set tokens (_swift_math_tokens -p)
if test -z (_swift_math_tokens -t)
set index (count (_swift_math_tokens -pc))
set tokens $tokens[..$index] \'\' $tokens[$(math $index + 1)..]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fish error: can't use the $(...) syntax, have to just use (...) instead:

Suggested change
set tokens $tokens[..$index] \'\' $tokens[$(math $index + 1)..]
set tokens $tokens[..$index] \'\' $tokens[(math $index + 1)..]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit ago, while testing fish with 4.0b1, I got an error using (…) instead of using $(…). I switched to $(…), which fixed the error.

After fixing the missing end problem (but not having worked on non-mutually-exclusive positionals & subcommands nor having made any other changes), I just verified that the both syntaxes offer helicopter, heliotrope & hello as completions for the following command line when completing with the cursor at ^ (which is not a literal caret, but just a marker to indicate the cursor location) on my macOS 12.7.6 Mac mini (2014) using fish 3.7.1 (which is the current stable version of fish) installed via Homebrew core:

command math stats quantiles --custom ^

What command & where in it did you try to complete? What version of fish? Etc.

I set this up by copying testMathFishCompletionScript().fish from the repo to ~/.config/fish/completions/math.fish, started fish 3.7.1 without any arguments, then ran:

fish_add_path /Users/username/Code/swift-argument-parser/.build/debug/

then completed at the end of the aforementioned command math stats quantiles --custom command line.

The math command had previously been built via swift test from zsh 5.9 installed via Homebrew core on the same Mac mini.

@rgoldberg
Copy link
Contributor Author

@natecook1000 Sorry, I forgot to make a change to the fish completion script (it currently incorrectly assumes that positionals & subcommands are mutually exclusive).

I'll work on the change, then push a new version of the PR.

I'll test it more thoroughly.

Might be later tonight or tomorrow.

Am sick (as of yesterday morning) & was too hastily trying to finish getting my changes merged that my look over was too lax.

@rgoldberg rgoldberg marked this pull request as draft February 17, 2025 02:16
@rgoldberg
Copy link
Contributor Author

@natecook1000 Some improvements that I've tried from fish 4.0b1 have some problems. I had messaged the fish people a bit about them, but it all fell by the wayside.

It seems like they might release 4.0 in a few weeks without any more prereleases after 4.0b1, so whenever I have their attention, I will concentrate on trying to get them to implement some some fixes.

I don't know Rust, nor do I know the fish codebase, so I probably cannot contribute code effectively to fish itself.

In the meantime, I will fix the issues in the fish PR as much as I can, but the eventual code might need to be different than the current paradigm to overcome fish shortcomings (e.g., I might need to tokenize the whole command line myself instead of using their standard tokenization), if fish 4.0 doesn't include certain improvements.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
If someone already escapes backslashes for such strings, this will cause an issue, but no one should be required to go to the trouble to manually escape backslashes in their strings, especially since the requirement isn't documented or normal.

If someone doesn't escape backslashes, then that can break then old version of the script.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
…ension.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Move from ArgumentDefinition extension to [ParsableCommand.Type] extension.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Fix custom completion of empty argument.

Associate a description with a fish completion iff it's a flag/option or a subcommand.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Resolve apple#737

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
… functions.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
…tion scripts.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
…ions as a separate argument.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Allow positionals & subcommands on the same (sub)command.

Don't use `-r` for complete calls for positionals.

Use `-\(r)fka ''` for positionals or option values with no completion candidates.

Allow option_specs elements to contain spaces.

Explicitly scope variables.

Rename functions.

Prevent odd characters in (sub)command names from breaking the script in some places.

Prevent missing data from breaking if tests.

Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
@rgoldberg
Copy link
Contributor Author

@natecook1000 Rebased on the new main.

Fixed the switch end issues.

Now support positionals & subcommands for the same (sub)command.

Other minor improvements.

Have been investigating improving other fish issues / asking fish to support more features in 4.0, but any changes for SAP will be in future PRs.

@rgoldberg rgoldberg marked this pull request as ready for review February 19, 2025 09:09
@natecook1000
Copy link
Member

natecook1000 commented Feb 20, 2025

@rgoldberg This is awesome, looks good!

Re compatibility – I was very confused at why this wasn't working for me locally – fish --version told me that I'm running version 3.6.0, which should support set -f (introduced in v 3.4.0) and the $(...) expansion syntax (likewise?), but it wasn't. It turns out that my $PATH pointed to a different version of Fish than my chsh setting, which was still on version 3.3.0! Before I figured that out, I made a small patch that got things working locally again, which you can view here: 854c36c. The changes seem relatively small, so if they still look future compatible, perhaps we can take them for now. That said, you'll know better than I do the implications of supporting back this far, so I can defer on that point; version 3.3.0 is nearly five years old.

@rgoldberg
Copy link
Contributor Author

@natecook1000 Thanks for investigating.

The safest way to check the version of fish that you're running is from fish itself, using:

echo $FISH_VERSION

I might not have time to get to testing your compatibility changes before I go away for the weekend.

-f -> -l is fine.

$(…) -> () will likely fail in fish 4.0b1. It might be a bug, though, so maybe it's already fixed on their main or could be fixed soon. If it's an intentional break, then I luse a fish version switch wherever that construct is used, or we could just only support fish from the first version on which the 4.0-compatible syntax works.

FYI: I have migrated fish to ToolInfoV0 locally, except that the default help subcommand is missing the --version flag (as was expected & as also occurs for bash) & that multiple flag ArgumentDefinitions are coalesced into one ArgumentInfoV0 with multiple long names. I haven't had a chance to fix that, or to even determine if ToolInfoV0 should be changed or if the fish completion code should accommodate the new setup. That probably must wait until next week.

Please note that there are numerous extant issues with all the shells. After the ToolInfoV0 refactor & other queued changes have been merged, I'll try to compile a list of verified or potential problems, then post them to #679 (or to separate issues), though if fixing an issue will take around the same time as documenting it, I'll just fix & submit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve fish completion script generation
2 participants