Skip to content

Replace the cursor index within the completing word parameter of custom-completion closures with a String completion prefix parameter #770

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

Merged
merged 2 commits into from
May 19, 2025

Conversation

rgoldberg
Copy link
Contributor

Replace the cursor index within the completing word parameter of custom-completion closures with a String completion prefix parameter.

Resolve #769

@natecook1000 @rauhul I think that this is an urgent improvement.

The Swift substring API is exceedingly verbose, and this will make using custom completions much easier.

We don't want to release the current API with the 2 indices to users because then it will need to be supported indefinitely, despite being suboptimal.

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 rgoldberg force-pushed the 769-completing-prefix branch from b9565bb to 936e5fa Compare May 13, 2025 12:25
@rgoldberg
Copy link
Contributor Author

@natecook1000 @rauhul I also have a PR for #555 (supporting async functions as custom completion handlers). I will submit it after this PR has been merged because it is branched from this PR's branch. It's not large at all, so shouldn't take long to review.

Please don't release a new version of Swift Argument Parser without checking with me first, because I might have other useful submissions queued up. Obviously, if I don't respond in a timely manner, go ahead & release new versions whenever you want, but if you could check with me first, I'd appreciate it, and users might get more functionality in the new release. Most importantly, if I realize that a new interface is suboptimal, it should be cleaned up ASAP before being officially released so it won't need to be maintained indefinitely, and so it won't be unpleasant for users.

Thanks again.

@natecook1000
Copy link
Member

@rgoldberg This change makes sense, since the conversion from Int to substring can be error-prone. Just to make sure I'm understanding this correctly: if a user has the following text on the command line, with their cursor as marked:

$ example --foo 1 --bar 2 baz bizzle
                           ^ cursor here, on 'a' in 'baz'

...then the parameters passed to the custom completion closure would be as if it were called like:

complete(["example", "--foo", "1", "--bar", "2", "baz", "bizzle"], 5, "b")

Is that right?

Comment on lines 468 to 473
completingArgument.prefix(
upTo: completingArgument.index(
completingArgument.startIndex,
offsetBy: cursorIndexWithinCompletingArgument
)
)
Copy link
Member

Choose a reason for hiding this comment

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

There's a prefix overload that just takes the count of elements to include, so this can be simplified to .prefix(cursorIndexWithinCompletingArgument).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the PR now. I'm still new to Swift, so my API knowledge is limited. I didn't even realize how verbose it is to get substrings in Swift. Thanks for pointing this out. All similar tips will be greatly appreciated, as it will produce much nicer code & get me up to speed much faster. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@natecook1000 Pushed the fix. Thanks again for the tip.

@rgoldberg
Copy link
Contributor Author

@natecook1000 My zsh cursor is always displayed between characters, not on them. Maybe you can switch cursor modes, but for the cursor in-between character mode, it would be between the b & the a. Typing backspace would delete the b. Typing delete would delete the a. Typing a normal character would insert it between the 2. In that case, the arguments you listed are correct. (sorry to be pedantic, but I want to ensure that I am as unambiguous as possible).

Thanks for looking into this.

rgoldberg added 2 commits May 18, 2025 19:02
…om-completion closures with a `String` `completionPrefix` parameter.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg rgoldberg force-pushed the 769-completing-prefix branch from 5b3a864 to f5ebfb5 Compare May 18, 2025 23:03
@natecook1000
Copy link
Member

@swift-ci Please test

@rgoldberg
Copy link
Contributor Author

The failing API breakage check should be OK because the replaced API was never released in a new version.

@natecook1000
Copy link
Member

@rgoldberg We're on the same page! The default cursor indicator for me is a block, even though I think of that as the cursor for overwrite mode (though I can't remember what platform that was on). In any case, you're describing what I meant to.

And exactly, merging over the API breakage check.

@natecook1000 natecook1000 merged commit 151e5ec into apple:main May 19, 2025
26 of 27 checks passed
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.

Replace second index parameter of custom-completion closures with a String prefix parameter
2 participants