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

Incorrect synopsis for char[] options #1834

Closed
rsenden opened this issue Oct 3, 2022 · 3 comments · Fixed by #1838
Closed

Incorrect synopsis for char[] options #1834

rsenden opened this issue Oct 3, 2022 · 3 comments · Fixed by #1838
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Milestone

Comments

@rsenden
Copy link
Contributor

rsenden commented Oct 3, 2022

I have some password options defined like the following:

@Option(names = {"--password", "-p"}, interactive = true, echo = false, arity = "0..1", required = true)
@Getter private char[] password;

Likely due to this option being defined as an array, the command synopsis in help output and man page show this as a repeatable option:

-p[=<password>] [-p[=<password>]]...

This is confusing for users, as it makes them think they can specify multiple passwords, whereas in reality they can only specify a single password consisting of multiple characters.

@remkop remkop added type: enhancement ✨ theme: usagehelp An issue or change related to the usage help message labels Oct 6, 2022
@remkop
Copy link
Owner

remkop commented Oct 6, 2022

I see, yes.
The logic for that lives in CommandLine.Help#concatOptionText.

Current:

                if (option.required()) { // e.g., -x=VAL
                    text = text.concat(name).concat(param).concat("");
                    if (option.isMultiValue()) { // e.g., -x=VAL [-x=VAL]...
                        text = text.concat(" [").concat(name).concat(param).concat("]...");
                    }
                } else {
                    text = text.concat("[").concat(name).concat(param).concat("]");
                    if (option.isMultiValue()) { // add ellipsis to show option is repeatable
                        text = text.concat("...");
                    }
                }

potential fix:

                // related: Interpreter#getActualTypeConverter special logic for interactive char[] options... (also: #648)
                boolean treatAsSingleValue = char[].class.equals(option.type()) && option.interactive(); // #1834

                if (option.required()) { // e.g., -x=VAL
                    text = text.concat(name).concat(param).concat("");
                    if (option.isMultiValue() && !treatAsSingleValue) { // e.g., -x=VAL [-x=VAL]...
                        text = text.concat(" [").concat(name).concat(param).concat("]...");
                    }
                } else {
                    text = text.concat("[").concat(name).concat(param).concat("]");
                    if (option.isMultiValue() && !treatAsSingleValue) { // add ellipsis to show option is repeatable
                        text = text.concat("...");
                    }
                }

Will you be able to provide a unit test?

@rsenden
Copy link
Contributor Author

rsenden commented Oct 6, 2022

I'll look into providing a PR with fix and unit test either myself or by my colleague.

@remkop
Copy link
Owner

remkop commented Oct 16, 2022

Looking at this more closely, I think #1846 (treating char[] as single value type) is a better solution. I will make the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants