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

More helpful error message on parameter order #13737

Merged
merged 1 commit into from Sep 23, 2015

Conversation

Projects
None yet
3 participants
@jasontedor
Member

jasontedor commented Sep 23, 2015

This commit addresses a confusing error message that arises when a
property parameter (e.g. -D) is after a double-dash parameter. The
current error message reports to the user that the parameter does not
start with “--". Adding the second dash as the error message suggests
causes the parameter to be silently ignored. This is confusing for the
user. With this commit, the user is now informed that the parameter
order is violated.

Relates e27ede4

throw new IllegalArgumentException("Parameter [" + arg + "]does not start with --");
if (arg.startsWith("-D") || arg.startsWith("-d") || arg.startsWith("-p")) {
throw new IllegalArgumentException(
"Parameter [" + arg + "] starting with \"-D\", \"-d\" or \"-p\" must be before any parameters starting with --"

This comment has been minimized.

@clintongormley

clintongormley Sep 23, 2015

Member

be -> appear?

@clintongormley

clintongormley Sep 23, 2015

Member

be -> appear?

This comment has been minimized.

@nik9000

nik9000 Sep 23, 2015

Contributor

I like be a little more in this case.

@nik9000

nik9000 Sep 23, 2015

Contributor

I like be a little more in this case.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

LGTM.

I'd probably add a test for this to BootstrapCliParserTests.

Contributor

nik9000 commented Sep 23, 2015

LGTM.

I'd probably add a test for this to BootstrapCliParserTests.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

Added 2.1 and 2.0 to this - I think its worth getting a better error message in for 2.0.

Contributor

nik9000 commented Sep 23, 2015

Added 2.1 and 2.0 to this - I think its worth getting a better error message in for 2.0.

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Sep 23, 2015

Member

@nik9000 I added a unit test in 72b67d17a90049d0aea4fa325922d814d4d59ba9.

Member

jasontedor commented Sep 23, 2015

@nik9000 I added a unit test in 72b67d17a90049d0aea4fa325922d814d4d59ba9.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 23, 2015

Contributor

LGTM.

Contributor

nik9000 commented Sep 23, 2015

LGTM.

More helpful error message on parameter order
This commit addresses a confusing error message that arises when a
property parameter (e.g. -D) is after a double-dash parameter. The
current error message reports to the user that the parameter does not
start with “--". Adding the second dash as the error message suggests
causes the parameter to be silently ignored. This is confusing for the
user. With this commit, the user is now informed that the parameter
order is violated.

Relates e27ede4

jasontedor added a commit that referenced this pull request Sep 23, 2015

Merge pull request #13737 from jasontedor/cli-parameter-order-error-m…
…essage

More helpful error message on parameter order

@jasontedor jasontedor merged commit 8d1d8f9 into elastic:master Sep 23, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor deleted the jasontedor:cli-parameter-order-error-message branch Sep 23, 2015

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Sep 23, 2015

Member

Thanks for reviewing @clintongormley and @nik9000. This is integrated into master, 2.0 and 2.x.

Member

jasontedor commented Sep 23, 2015

Thanks for reviewing @clintongormley and @nik9000. This is integrated into master, 2.0 and 2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment