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

Provide completions for s3cmd #4332

Closed
wants to merge 3 commits into from

Conversation

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Aug 15, 2017

No longer auto-generated. Everything has been summarized. Supressing
file completions for initial command, providing list of valid initial
commands, filtering --options by subcommand.

No longer auto-generated. Everything has been summarized. Supressing
file completions for initial command, providing list of valid initial
commands, filtering --options by subcommand.
@mqudsi mqudsi force-pushed the mqudsi:s3cmd_completions branch from 9193a36 to 497f40a Aug 15, 2017
# s3cmd

function __s3cmd_is_remote_path
commandline -pct | string match -r -- "^s3://.*"

This comment has been minimized.

@faho

faho Aug 18, 2017
Member

You'll usually want to use the "-q" flag for string match.

It's okay here since this is only used as a complete condition, which has its output already silenced.

function __prev_tokens
set __tokens (commandline -pco)
for arg in $argv
if echo $__tokens[-1] | string match $arg

This comment has been minimized.

@faho

faho Aug 18, 2017
Member

string match -q -- $argv $__tokens[-1] - the echo is unnecessary.

This comment has been minimized.

@faho

faho Aug 18, 2017
Member

Also, this is basically __fish_seen_subcommand_from.


function __initial_command
set __tokens (commandline -pco)
string match (count $__tokens) 1

This comment has been minimized.

@faho

faho Aug 18, 2017
Member

This is a bit unorthodox.

Strictly, this would be set -q __tokens[1]; and not set -q __tokens[2]. You could probably get by without the first check, depending on what exactly you want to do with this.

Alternatively, test (count $__tokens) = 1.

This comment has been minimized.

@faho

faho Aug 18, 2017
Member

There is also __fish_is_first_token already. That one skips options.

Personally, for subcommands, I prefer not __fish_seen_subcommand_from $cmds (see e.g. the systemctl completions). That allows for options and option-arguments, as long as they don't look like a command.

For a completely correct solution, you're going to need to do something like the git completions, or use argparse (which I'm converting both the git and systemctl completions to).

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Aug 18, 2017

@faho thanks for reviewing; I knew there would be a significant amount of feedback if I opened this a PR rather than merging directly despite the fact that it works.

I had a hard time finding a good reference on the best practices for writing completions and each completion file I looked at was different than the other (though of course I didn't go through too many). Perhaps we can add a more-indepth article about writing completions to the online resources, as the existing seemed to only cover the very basic usage of complete.

I didn't look at the completions for sysctl, I tried to base mine off of apt-get but found it too hard to follow and so ended up reinventing the wheel :)

It doesn't seem like it should be too hard to take these suggestions into account (I'm going to hold off on argparse at this time, if it's OK with you); let me review and push.

@faho
Copy link
Member

@faho faho commented Aug 18, 2017

I had a hard time finding a good reference on the best practices for writing completions and each completion file I looked at was different than the other

Yes, they are. They were written at different times by different people. Obviously I prefer my own style.

Perhaps we can add a more-indepth article about writing completions to the online resources

That would be nice to have, yes.

I didn't look at the completions for sysctl

Note: systemctl, which is basically systemd's main command. sysctl is that thing for tweaking kernel knobs.

The completions for it are nice (except for the use of eval), and easier to follow than the massive git script.

I'm going to hold off on argparse at this time, if it's OK with you

Sure.

let me review and push.

Yes. You can push directly to Integration_2.7.0 if you want - no need to review again, unless you prefer that. In its current state this is okay, my objections are stylistic.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Aug 18, 2017

OK, thanks again.

…tions

Per the discussion with @faho in #4332, replaced some custom completion
state detection functions with standard __fish_* functions used in other
completion sources.
mqudsi added a commit that referenced this pull request Aug 18, 2017
…tions

Per the discussion with @faho in #4332, replaced some custom completion
state detection functions with standard __fish_* functions used in other
completion sources.
@mqudsi mqudsi closed this Aug 18, 2017
@zanchey zanchey added this to the fish 2.7.0 milestone Aug 19, 2017
krader1961 added a commit that referenced this pull request Aug 19, 2017
…tions

Per the discussion with @faho in #4332, replaced some custom completion
state detection functions with standard __fish_* functions used in other
completion sources.

(cherry picked from commit f706081)
@mqudsi mqudsi deleted the mqudsi:s3cmd_completions branch Oct 2, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.