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

Print help once on invalid command line arguments #1376

Merged
merged 2 commits into from Dec 12, 2015

Conversation

Projects
None yet
3 participants
@nickbabcock
Contributor

nickbabcock commented Dec 7, 2015

Currently, if -h is provided on the commandline and invalid arguments are specified as well, the help message will be printed twice. This is not desired (the help message should only be printed once)

This change is not backwards compatible for those who need to see that is-help is set in the run command (this would only be when the case when all arguments are optional)

Closes #1357

@jplock jplock added this to the 1.0.0 milestone Dec 7, 2015

@jplock jplock added the improvement label Dec 7, 2015

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 8, 2015

Just wanted to give a heads up that the implementation uses HelpScreenException in the 'internal' package of argparse4j. This matches the help argument action in argparse4j, which is the same as the one in dropwizard, except argparse4j doesn't take an output to write the help to. So even though, this implementation uses a class in the internal package, I think it is the best implementation.

}
return true;
} catch (HelpScreenException e) {

This comment has been minimized.

@arteam

arteam Dec 9, 2015

Member
  • Could we make a comment why true is returned? From the first glance it's not obvious that HelpScreenException is not an error.
  • Maybe better to name the variable ignored instead of e? I beleive that is a common name for ignored exceptions.
}
};

private final JarLocation location = mock(JarLocation.class);

This comment has been minimized.

@arteam

arteam Dec 9, 2015

Member

Could we make location a local variable in the setUp method? I believe we don't use it in the test method(s).

};

private final JarLocation location = mock(JarLocation.class);
private final Bootstrap<Configuration> bootstrap = new Bootstrap<>(app);

This comment has been minimized.

@arteam

arteam Dec 9, 2015

Member

Ditto, It could be a local variable in the setUp method.

private Cli cli;

@Before
@SuppressWarnings("unchecked")

This comment has been minimized.

@arteam

arteam Dec 9, 2015

Member

Do we need this annotation? I believe all operations are type-safe in the method.

when(location.getVersion()).thenReturn(Optional.of("1.0.0"));
bootstrap.addCommand(command);

this.cli = new Cli(location, bootstrap, stdOut, stdErr);

This comment has been minimized.

@arteam

arteam Dec 9, 2015

Member

Probably no need for this, because we don't have a local variable with the same name.

@arteam

This comment has been minimized.

Member

arteam commented Dec 9, 2015

Looks good! 👍
I've added a couple nitpicks, but overall this is ready to merge.

@nickbabcock nickbabcock force-pushed the nickbabcock:help-exception branch from 82ea8ad to b849a68 Dec 9, 2015

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Dec 9, 2015

Looks like the nitpicks arose because I was too eager in copying and pasting the setup code from CliTest 😊

Anyways, pr should be good to go.

arteam added a commit that referenced this pull request Dec 12, 2015

Merge pull request #1376 from nickbabcock/help-exception
Print help once on invalid command line arguments

@arteam arteam merged commit e363c0b into dropwizard:master Dec 12, 2015

@arteam

This comment has been minimized.

Member

arteam commented Dec 12, 2015

Thanks for your contribution!

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