-
Notifications
You must be signed in to change notification settings - Fork 332
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
Completion problems #679
Comments
CompletionKind
s are not properly implemented for some shells
I am partway through a PR for this, with many improvements for all 3 shells. What is the oldest version of each shell with which the respective script must maintain compatibility? Here are some guesses:
I know that SAP can be used on OSes other than macOS (sorry to be Mac-centric, but I don't know shell versions distributed with / popular for other OSes). Supporting bash 3.2.57 might be annoying because it doesn't support associative arrays. Being able to require some bash 4.x or (better yet) 5.x might simplify things. |
If we can make things work with the default-installed Bash version, I think we should. I know it's a bit of a pain, but it would feel like a mismatch (and probably generate more issues) to require Bash v4 or v5. The other two shell versions both seem reasonable! |
@natecook1000 thanks. Will do. 2 questions: Overloaded Swift custom completion functionsI think there should be 2 extra arguments to Swift custom completion functions:
I would leave the existing single-argument function as is but deprecate it, and add & recommend the new three-argument overload. Completion inconsistencies between shellsThere are inconsistencies amongst how the completions behave in the 3 shells. Is it OK if I make all 3 behave as similarly as possible? I'll try to document all inconsistencies that I couldn't overcome within a reasonable amount of effort. I will standardize around what I perceive to be the best completion behavior. In most cases, this will be closest to the current zsh behavior. I'll attempt to note all behavior changes. Most inconsistencies seem to be contained within the shell completion behavior itself, so fixing them should be backwards compatible with existing SAP uses. The 3 shells, however, pass the command-line words to Swift custom completion functions in varied ways. Making that info consistent, and fixing bugs for which people might already workarounds, could break existing Swift custom completion functions. See below for more info. |
@natecook1000 Thanks for merging my 2 other PRs. Here some of the problems I'm fixing. There are many more issues, but the following are ones about which I have open questions. Should I include anyone else in this discussion? Backwards compatibilityThe way that Frequently, they aren't properly escaped (so the whole completion script can break; Swift either must not include certain values as completions, or must escape things properly in some places, but not escape them in others, differently for the different shells, etc.), but sometimes they are, so devs never know what they must do. Some Etc. Devs might have worked around all these bugs, but many workarounds won't work properly after my PR fixes all these problems. It will be easiest to not try to be compatible with the old, incredibly broken way of handling Custom completion command line contentsSimple questionShould the shells pass the full command line to Swift custom completion functions? That would include other commands (separated by If Swift shouldn't receive the full command line, what should mark things to excise? I assume DetailsI have found multiple inconsistencies & problems with how the contents of the current command line are supplied to Swift custom completion functions by the 3 supported shells. Undiscovered inconsistencies & problems might exist. None of the shells supplies all words from the command line that we should want shells to pass to Swift custom completion functions. We will unfortunately need to supply verbatim (instead of unquoted) words to Swift; otherwise, e.g., My upcoming PR will support 2 types of custom functions:
We could try to keep the broken existing behavior in the deprecated function, but there are so many other problems with completions that will require breaking fixes that we probably want to bump major version, and fix the problems in the deprecated function, too. In examples:
Table detailing how the command line is communicated from shells to Swift custom completion functions:
Footnotes
|
Testing platforms for custom completion command line contentsThe above results were obtained on macOS for the following shell versions:
All versions of a shell behaved the same, unless explicitly noted. Will try to test zsh 5.3, bash 4.0, and any shell versions released from now until when the PR is merged. Should I test any other shell versions? I do not have any OSes other than macOS handy. If testing must be performed on other OSes (or on multiple versions of macOS), can you find someone else to handle them? |
Questions about custom completion command line contents
|
Swift completion outputAfter my upcoming PR, all completion candidates supplied from Swift must be output as the verbatim text for the equivalent of a zsh single-quoted string (i.e. no escape values are supported), except that single quotes may be used within the output values. For shells (like bash & zsh) that support verbatim single-quoted strings (no escapes, single quotes not allowed as direct content), SAP will properly replace each single quote in the content by splitting the output into multiple single-quoted strings concatenated with escaped single quotes between them. For shells (like fish) that do not support verbatim single-quoted strings, SAP will properly escape all necessary characters (e.g., for fish shell, For There are many problems with how SAP 1.5.0 parses completions from Swift. Some (if not all) types of completions in the different shells split completions on spaces (so It's so broken, I think it makes sense to only follow the consistent, fixed rules that will be followed by my PR (and will be considered bugs if not obeyed). This might technically require SAP to bump by a major version. |
Index issues: fish 3-: words are unquoted, but "cursor index in current word" is based on the verbatim characters, so it could be greater than it should be. Fixed in fish 4+. If someone else wants to improve this for fish 3-, they can. zsh: "cursor index in current word" is one greater than correct if the cursor is immediately after a backslash used as an escape (i.e. zsh reports the cursor as after the character that is escaped by the backslash, instead of after the backslash itself). |
In what format do you want text to be passed between shells & Swift in the various places where they can communicate? The new The following ignores environment variables as a communication method between shells & Swift. Anyone using them can do whatever they want with them. It's also what I've gathered going through this all. If anything is incorrect, please let me know. From shell to SwiftText is sent from a shell to Swift only for custom completions, which pass the contents of part of the command line from the shell to Swift. In this case, we must send verbatim text, otherwise there would be no way to distinguish between:
Are there any bash, fish, or zsh parsing libraries in Swift that we can recommend to users? From Swift to shellText sent from Swift to a shell should probably be expected to be in different formats depending on the use case. The only places about which I know where Swift sends text to a shell are in the completion functions, and in the command, subcommand, flag & option names. Completion functions:
Command, subcommand, flag & option names should behave like [-+._:0-9A-Za-z] I have seen |
In what order should arrays of completions be shown? The two main options would be
I think the former is mainly (if not exclusively) what is done in 1.5.0, but I prefer the latter. If you want your completions to be sorted a certain way, the latter provides flexibility, while the former is restrictive. |
@rgoldberg Can't thank you enough for your investigation and work on these issues! I've gone section by section through the above -- please let me know if you're blocked on my responses and we can discuss further. Overloaded Swift custom completion functionsBoth those points look reasonable to me, as long as the overload and deprecation aren't source-breaking. Let's make sure that we include tests that validate that the single-parameter function is still usable. Providing two Completion inconsistencies between shells
Absolutely – this is a great goal across the board. Backwards compatibility
Let's take a look at how breaking these changes are as we land them. My expectation is that you will be fixing far more latent bugs in tools that rely on ArgumentParser than you will be breaking workarounds. Custom completion command line contentsIt makes the most sense to me that command-line tools only get the section of the command line that is relevant to that tool, if that is possible -- tools shouldn't get data that isn't intended for them. Your list of operator delimiters looks good for what marks the section -- does that work for you? Testing platformsMy main concern is about having a way to test these in an automated fashion – even that matrix is more than I want anyone (including/especially you) to have to check and validate manually. Have you used existing tools (like Questions about custom completion command line contents
Redirects are outside the scope of a command's execution, so I'd rather they not be included. Are there contexts where information about redirects necessarily needs to be relevant when generating completions? Swift completion outputI think I'll need to see the change in behavior to fully understand what you're describing in this section. Index issuesRe: fish 3 and zsh "cursor index in current word": Let's make sure we document the inconsistencies where we find them, if there isn't a reasonable fix that we can apply. From shell to Swift
I don't know of any - we could look at providing Swift wrappers for parsing/expansion/etc via each of the shells as a future enhancement. From Swift to shellOverall, I'd rather look at adding overloads that allow a more structured return type than finding delimiters to use in strings. Would it work to let people return something shell-agnostic that ArgumentParser could then format correctly for the destination shell? Re: limiting the character set – is this in completions, or in option/flag/argument names more broadly? The character set you've described looks good, but I think we'll need to be careful if we're imposing a limit that existing tools haven't had to deal with. Order of arrays of completionsAs far as I know, ArgumentParser isn't doing sorting of any kind – is that a behavior the shells are providing? I'm happy for it to be in the manually specified order if that's possible. |
@natecook1000 Thanks for the replies. I'll look over them soon. Can I start breaking out sets of fixes to submit as PRs? Should I make a separate issue for each PR? So I can coordinate PR sizes with the reviewer, will you or will someone else review my PRs? |
@natecook1000 Thanks again for feedback. I'll get back to you if I run into anything else for which you haven't already supplied answers.
I'm happy to do whatever you want. Would the tuple or struct also include the
See my answer below about redirects for a discussion about why including redirects & potentially pipes (at least incoming ones) might be useful. If we do include pipes (& potentially other command-line segments), we might want to include 2 more indices: the index of first word of current command, and (one more than?) the index of last word of current command (or just the word count for the current command). Even if I can't think of reasons to include other command-line segments right now, someone else might have a reason at some point. The more info we provide, the less likely we are to leave something out that someone needs. If someone writes a script by hand, they have access to the full command line, so why not provide the whole thing to Swift
I haven't looked into automating this. I just did my tests manually as I needed to investigate how scripts interacted with their respective shell. I'd imagine that the best way to automate testing for all this would be to test all builtin shells from macOS on each macOS version for which there is a GitHub runner image, the latest stable version of each of the shells (obtained via Homebrew), and designated versions of shells (I don't currently know how those designated versions would be installed). I imagine that there'd be tests that start a command line, enter text, enter a TAB, then scrape the stderr (or stdout, as appropriate) for the completions. It's all a bit hand wavy, as I imagine that any such automated testing wouldn't be implemented until after all the fixes are in.
I would imagine that an in redirect (or an incoming pipe) might be useful to know because some commands require input either from stdin or from a file specified by a path. If you already have the redirect (or pipe), completions for an input file path shouldn't be offered. I don't think there's an equivalent situation for out redirects (or outgoing pipes), however, because in the absence of an out redirect (or pipe), the output will go to the console. (Am I missing anything?) If the above is correct, there would be an inconsistency if only including in redirects but not incoming pipes, and an asymmetry if only including in redirects & pipes but not out redirects & pipes. The various shell scripts currently all omit pipes from the sources that we use for command lines, but they handle redirects differently. I think we can get the full command line from each of the shells, so we can always manually select whatever we want, but that will take more time, be more verbose, more error-prone, and likely more brittle for forward compatibility (e.g., a shell could introduce a new symbol that we must parse out).
Sorry, please ignore that section; my proposals therein were inadvisable. I went over the same issues elsewhere with better solutions. My general principle is for Swift to receive & to output text without having to know shell syntax as much as possible, but to use shell syntax where necessary. This can now be done thanks to the 2 singletons I recently introduced. My forthcoming PRs will additionally ensure that malformed shell text from users will not break scripts outside of the individual completion for which they were specified (e.g., for some completions for some shells, a quote in a completion will completely break the generated script rather than just result in that one completion not working).
For IIRC, We could avoid delimiters by using environment variables, but that would get much more complex than just using either NUL (i.e. Unicode code point 0; not the 3 letters I can't imagine either NUL or newline being valid characters within a completion; newline would be simpler to use than NUL. If we use one of those characters as a delimiter, it shouldn't be supported by any other completion, so it should be safe to use for Do you agree that we should use some delimiter? If so, can we use newline & NUL as appropriate (NUL might be simpler/better than newline in some places, and vice versa), or must I use NUL? (I can't imagine that newline would be able to be used but not NUL, so, unless you disagree, I expect to use just NUL, or to choose between NUL & newline as convenient)
I think that, in the long run, the completion scripts should support all valid flags, options, and (sub)commands. Currently, I'm sure that various characters will break the completion scripts (probably spaces, newlines, special shell characters (e.g.,
IIRC, at least some of the shell scripts sort some of the completions. I think that |
Partial apple#679 Signed-off-by: Ross Goldberg <[email protected]>
There are numerous completion problems: some only for certain shells, some for specific
CompletionKind
s for certain shells, and some that are only for arguments but not for option values.bash
--
directory
not implemented for argumentsfile
not implemented for argumentsdirectory
&file(…)
for option values moves to next parameter after selecting the first item, even if you've submitted a directory, instead of allowing you to request candidates from within the selected directories. If you move back to the parameter & append a/
, tabbing will then offer properly filtered candidates.There might not be a way in bash
3.2.57
, or even in later bashes, to improve the following:fish
-
file()
offers no candidates when no extensions were specifiedfile(extensions:)
doesn't offer directorieszsh
shellCommand
does not offer any candidatesThere aren't necessarily bugs, they are just potentially suboptimal setups / aren't properly documented in the docs or code:
-
, doesn't offer flags or options if there are unsupplied positional argumentsfile(extension:)
if existing part of the completing parameter is a path to a leaf directory, and if no non-hidden files in that directory match any of the given extensions, then all non-hidden files in that directory are offered as candidates, regardless of their respective extensions. Is that intended? If so, it should be documented.all
The following
@Option
doesn't offertrue
&false
. Using a@Flag
would probably be better, but if a developer wants to use aBool
@Option
, the completion scripts should probably offertrue
&false
without additional setup being required.Documentation
The
CompletionKind
/CompletionKind.Kind
documentation is also sometimes wrong or confusing, so I will update that along with the associated code.PR
I will work on a PR to fix these problems. If I find additional problems, I'll add them to this issue.
Related Issues
CompletionKind
should support multiple options #566: My PR will fix the problems mentioned, but won't implement the proposed solution.Versions
ArgumentParser version:
main
branchSwift version:
Checklist
main
branch of this packageSteps to Reproduce
Generate completion scripts using the aforementioned
CompletionKind
s.Try to use them.
Expected behavior
Completion works.
Actual behavior
Some completion either does nothing, or outputs an error.
See per-shell sections above for details.
The text was updated successfully, but these errors were encountered: