Skip to content

Improve fish completion (#376, #534)#535

Merged
natecook1000 merged 5 commits into
apple:mainfrom
mtj0928:fix-fish-completions
Jan 9, 2023
Merged

Improve fish completion (#376, #534)#535
natecook1000 merged 5 commits into
apple:mainfrom
mtj0928:fix-fish-completions

Conversation

@mtj0928
Copy link
Copy Markdown
Contributor

@mtj0928 mtj0928 commented Dec 17, 2022

A generated fish script didn't complete an option after an argument.

The cause is the generated fish script doesn't accept an input text which already has arguments.
For example, when the input is repeat -, the script can complete --count, but when the text is repeat foo -, the script cannot complete repeat foo --count, because the input text already has the argument "foo".

To fix the issue, I implemented the following steps.

  1. Preprocess for the subcommand.
    To support options of the root command like math --version add, --version is removed by _swift_{command name}_preprocessor.

  2. Decide a command.
    In a case when a command has some subcommands like math stats average 1 10 100 --kind mean, we need to decide the target command. (In the example case, the target command is average.)
    To decide it, the newly generated script receives expected command and subcommands.
    (e,g,)

    1. A case checking whether the target command is stats or not.
      • expected command = math stats
      • subcommands = average stdev quantiles help
    2. A case checking whether the target command is average or not.
      • expected command = math stats average
      • subcommands is empty (average doesn't have any subcommands.)`

    When the input has the expected command, and a text after the expected command is not the subcommand, the newly generated script decide the command is a target command, and suggest options for the target command.

Related issues

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

A generated fish script didn't complete an option after an argument.

The cause is the generated fish script doesn't accept an input text which already has arguments.
For example, when the input is `repeat -`, the script can complete `--count`, but when the text is `repeat foo -`, the script cannot complete `repeat foo --count`, because the input text already has the argument "foo".

To fix the issue, `FishCompletionsGenerator` got a capability that can accept a text which has arguments.
Copy link
Copy Markdown
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.

Thanks so much, @mtj0928! Just a couple of small changes below, and could you update the fish test expectation for testMath_CompletionScript()?

results.append((commandChain, suggestion))
}
}
private enum FishScriptBuilder {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need a new namespace here — can these just be fileprivate static members of FishCompletionsGenerator?

Comment on lines +66 to +67
var results = names
.map{ $0.asFishSuggestion }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
var results = names
.map{ $0.asFishSuggestion }
var results = names.map { $0.asFishSuggestion }

@mtj0928
Copy link
Copy Markdown
Contributor Author

mtj0928 commented Jan 8, 2023

@natecook1000 Thank you for your review!
I applied your comment, and please review this PR again!

Copy link
Copy Markdown
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.

LGTM!

@natecook1000
Copy link
Copy Markdown
Member

@swift-ci Please test

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.

2 participants