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 · 5 comments · May be fixed by #2388
Open

scriptName breaks completion script if it contains whitespace #2387

davidmurdoch opened this issue Jan 12, 2024 · 5 comments · May be fixed by #2388
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 linked a pull request Jan 12, 2024 that will close this issue
Open
@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

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}.

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
2 participants
@davidmurdoch @shadowspawn