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

Automatically show help when requested even if parsing fails #53

Closed
rvesse opened this issue Sep 15, 2016 · 3 comments
Closed

Automatically show help when requested even if parsing fails #53

rvesse opened this issue Sep 15, 2016 · 3 comments
Assignees
Labels
bug Bug enhancement Proposed Enhancement/Feature
Milestone

Comments

@rvesse
Copy link
Owner

rvesse commented Sep 15, 2016

Currently if parsing fails for whatever reason we throw an exception so any logic that a developer might have written to provide help for the command is most likely ignored. Also because they won't have received an instance of their command class even if they had used something like the HelpOption and the user specified --help at the command line they have no way of inspecting this value short of manually inspecting the command line inputs.

In some of the existing places where I use airline currently I end up creating special logic to do just this when really it's something that the library should do automatically.

It would also be nice if there is a way to get all the parser Errors in one go and present these to the user.

This problem dates all the way back to the original library and one possible solution was suggested there but was never actually adopted. See airlift#22

Given all the changes that have happened since then that approach almost certainly won't directly apply onto the current code but the general approach should work. I would be tempted to actually generalise this further and have a @SupressErrors or similar annotation/arguments to annotations that could be used to indicate that the presence of an option should suppress throwing errors. Also given that we have a configurable parsing infrastructure we may wish to make the behaviour configurable:

  • always throw when first error is encountered
  • throw all errors at end of parsing as a single compound error
  • suppress errors if specific options are provided

The best thing to do might be to define an enumeration of possible behaviours and provide that as a field on the @Parser annotation and then improve the parser to behave accordingly.

@rvesse rvesse added bug Bug enhancement Proposed Enhancement/Feature labels Sep 15, 2016
@rvesse rvesse modified the milestones: Unscheduled, 2.3.0 Sep 15, 2016
@rvesse
Copy link
Owner Author

rvesse commented Nov 4, 2016

The following commit shows another variation on this approach:

jontodd@66d85bb

My inclination is to go with hybrid of the various approaches. Basically define a new ErrorHandler Interface and have the parsers route all errors through this via a ParseState.withError() method. We can then have several Basic implementations provided based on the behaviours already described. Yet this would also allow end users to define entirely custom logic as desired. It also may mean that we don't need a @SuppressErrors annotation or argument to options.

@rvesse
Copy link
Owner Author

rvesse commented Nov 4, 2016

One thing to be careful of if we allow for suppressing Errors is that when we suppress an error we should make sure that we have consumed the token that led to the error so that we can continue parsing and don't get stuck in an infinite loop. It may be possible to add logic to detect this case into our error handlers e.g. If the handler received an error which is of an identical type to the preceding error and the message and cause trees are equivalent we bailout and throw an error regardless

rvesse added a commit that referenced this issue Nov 25, 2016
Adds a new ParserErrorHandler interface and incorporates it into the
ParserMetadata.  Doesn't yet utilise it properly
rvesse added a commit that referenced this issue Nov 25, 2016
Modifies ParseState<T> and SingleCommandParser<T> to use
ParserErrorHandler and ParseResult where appropriate
rvesse added a commit that referenced this issue Nov 25, 2016
- Adopt in CliParser
- Move command instantiation logic to ParseResult<T>
@rvesse rvesse self-assigned this Nov 25, 2016
rvesse added a commit that referenced this issue Dec 1, 2016
Adopts use of ParserErrorHandler in other appropriate places
rvesse added a commit that referenced this issue Dec 1, 2016
Adds the showHelpIfErrors() method to HelpOption so a parsed command
with errors can access its HelpOption and call showHelpIfErrors().  If
there were errors then it goes ahead and shows the help (and optionally
the errors).
rvesse added a commit that referenced this issue Dec 1, 2016
Adds and documents parseWithResult() methods on SingleCommand and Cli
@rvesse
Copy link
Owner Author

rvesse commented Dec 1, 2016

Now implemented as follows:

  • Parser configuration takes a ParserErrorHandler interface
  • Any non-fatal parsing errors are handled by that interface
  • For backwards compatible behaviour defaults to FailFast implementation
  • New FailAll and CollectAll implementations that collect errors and either throw or store them
  • New ParseResult<T> class and parseWithResult() methods on SingleCommand and Cli
  • New HelpOption.showHelpIfErrors() which takes in a ParseResult and automatically shows help if any errors occurred

So showing help on errors is not automatic but is now much more easily done.

@rvesse rvesse closed this as completed Dec 1, 2016
rvesse added a commit that referenced this issue Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug enhancement Proposed Enhancement/Feature
Projects
None yet
Development

No branches or pull requests

1 participant