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.

Sign up for GitHub

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

Jump to bottom

scriptName breaks completion script if it contains whitespace #2387

Open
davidmurdoch opened this issue Jan 12, 2024 · 7 comments · May be fixed by #2422
Open

scriptName breaks completion script if it contains whitespace #2387

davidmurdoch opened this issue Jan 12, 2024 · 7 comments · May be fixed by #2422
Labels
bug

Comments

@davidmurdoch
Copy link

Reproduction:

One liner:

require("yargs").scriptName("yarn whatever").completion().parseSync(["completion"])

You can paste this into your terminal to get a quick reproduction:

mkdir scriptNameCompletionTest && cd scriptNameCompletionTest && npm install yargs --no-save >/dev/null && node -e 'require("yargs").scriptName("yarn whatever").completion().parseSync(["completion"])'

The output:

###-begin-yarn whatever-completions-###
#
# yargs command completion script
#
# Installation: yarn whatever completion >> ~/.bashrc
#    or yarn whatever completion >> ~/.bash_profile on OSX.
#
_yarn whatever_yargs_completions()
{
    local cur_word args type_list

    cur_word="${COMP_WORDS[COMP_CWORD]}"
    args=("${COMP_WORDS[@]}")

    # ask yargs to generate completions.
    type_list=$(yarn whatever --get-yargs-completions "${args[@]}")

    COMPREPLY=( $(compgen -W "${type_list}" -- ${cur_word}) )

    # if no match was found, fall back to filename completion
    if [ ${#COMPREPLY[@]} -eq 0 ]; then
      COMPREPLY=()
    fi

    return 0
}
complete -o bashdefault -o default -F _yarn whatever_yargs_completions yarn whatever
###-end-yarn whatever-completions-###

Notice the invalid bash function _yarn whatever_yargs_completions.

I think if scriptName is going to be used in the generated bash completion script it should be sanitized (https://stackoverflow.com/a/28115066/160173). Or completions should allow the use to specify their own function name.
@davidmurdoch davidmurdoch mentioned this issue Jan 12, 2024
Closed
@shadowspawn
Copy link
Member

I agree it is a short-coming, but under what circumstances would it come up in practice? A script with a space in the name seems unlikely. However, if you actually encountered this bug in real world then definitely of interest!

@davidmurdoch
Copy link
Author

davidmurdoch commented Jan 23, 2024 •
edited
Loading

One use is any file name with a space in it, which might not be common, but is valid everywhere.

But also if your yargs interface is a package.json script, e.g., yarn webpack, then it makes sense for --help, which uses the value of scriptName in its Commands, Usage, and Examples outputs, to show the actual command a user should use, i.e. yarn webpack [command], not some-file.js [command].

Example:

Confused users:

image

Happy users (except that the completions command doesn't actually work now):

image

Regarding the PR: I didn't know how to write an appropriate test with the test helpers in use here. Does it look okay as is? Anything else required?

@shadowspawn
Copy link
Member

The "run script" example is thought provoking! But wouldn't your PR install a completion definition for a command named yarn_webpack so still not match the supposed command displayed in the help?

@shadowspawn
Copy link
Member

Regarding the PR: I didn't know how to write an appropriate test with the test helpers in use here. Does it look okay as is? Anything else required?

The existing tests are in test/completion.cjs. I think they are all calling the completion generation code directly (i.e. using --get-yargs-completions) and not going through the shell process. You have added a test for the name sanitisation which I think is reasonable and sufficient.

@davidmurdoch
Copy link
Author

But wouldn't your PR install a completion definition for a command named yarn_webpack so still not match the supposed command displayed in the help?

No, there are 2 template strings in the completion bash function where the scriptName is used: {app_name} and {app_path}. {app_name} is used in the name of the function and the comments, {app_path} is used for the completion command itself. The linked PR only changes {app_name}.

@davidmurdoch davidmurdoch linked a pull request Jun 28, 2024 that will close this issue
Open
@shadowspawn
Copy link
Member

One use is any file name with a space in it, which might not be common, but is valid everywhere.

But also if your yargs interface is a package.json script, e.g., yarn webpack

These are two different and incompatible use cases. (I was having another look at this and realised conflating two different use cases.)

In the first case there is a command with a space in it. It gets invoked like:

'my build command' compile

In the second case there is a command with an argument:

yarn webpack

@davidmurdoch
Copy link
Author

In the implementation I didn't give any thought to the "command with a space" in it case. I don't think there are many developers that would think about designing a command name that has to be quoted to use it. It also doesn't work with yargs completion currently, so improving yargs completion to allow for the common "multiple argument command" case shouldn't affect it, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: ensure completion script uses valid function name davidmurdoch/yargs
fix: ensure completion script uses valid function name davidmurdoch/yargs
2 participants
@davidmurdoch @shadowspawn