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

Public subclasses of OptionException #58

Closed
andrewbts opened this Issue Apr 3, 2014 · 13 comments

Comments

Projects
None yet
3 participants
@andrewbts

andrewbts commented Apr 3, 2014

Any reason not to make the various subclasses of OptionException (such as MissingRequiredOptionException) public classes? That way we can catch and handle them individually.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Apr 4, 2014

Collaborator

Hi @andrewbts, thanks for your interest!

My thinking in keeping the subclasses of OptionException package-private was that an OptionException, no matter its leaf type, would pretty much be handled in the same way: show the exception's message on the console, maybe its stack trace, and exit. Because of this, the specific type wouldn't be important. So I decided to keep them package-private, thereby reducing the surface of the public (and published) API.

Also, keeping the specific types hidden (an implementation detail, if you will) affords us the freedom to rename/remove/refactor them without worrying about breaking callers.

I'm willing to reconsider this decision. I'm interested to hear what different strategies you'd use to handle different subclasses of OptionException. Do the messages in the exceptions not contain enough info? need internationalized?

Collaborator

pholser commented Apr 4, 2014

Hi @andrewbts, thanks for your interest!

My thinking in keeping the subclasses of OptionException package-private was that an OptionException, no matter its leaf type, would pretty much be handled in the same way: show the exception's message on the console, maybe its stack trace, and exit. Because of this, the specific type wouldn't be important. So I decided to keep them package-private, thereby reducing the surface of the public (and published) API.

Also, keeping the specific types hidden (an implementation detail, if you will) affords us the freedom to rename/remove/refactor them without worrying about breaking callers.

I'm willing to reconsider this decision. I'm interested to hear what different strategies you'd use to handle different subclasses of OptionException. Do the messages in the exceptions not contain enough info? need internationalized?

@andrewbts

This comment has been minimized.

Show comment
Hide comment
@andrewbts

andrewbts Apr 4, 2014

Hi, thanks for you reply. I did proceed as you had anticipated: Printing the message in the OptionException, print the help, and exit.

What I was planning on doing, motivating me to create this Issue, was:

  • Catch MissingRequiredOptionException: emit my own message, print the help, and exit.
  • Catch any other exception: die with full stack trace because I really wasn't expecting that.

But I think considering the goals of the library the current behavior is fine.

One thing that troubled me (not enough to change it :) ) is that the message for missing required options includes all synonyms of the missing required options:

Here I have two options

  • "p" or "place"
  • "d" or "data-dir"

The message:
"Missing required option(s) ['d', 'p', 'place', 'data-dir']"

Not easy to see how to get around it through, without introducing some notion of the "preferred" form/name for an option.

andrewbts commented Apr 4, 2014

Hi, thanks for you reply. I did proceed as you had anticipated: Printing the message in the OptionException, print the help, and exit.

What I was planning on doing, motivating me to create this Issue, was:

  • Catch MissingRequiredOptionException: emit my own message, print the help, and exit.
  • Catch any other exception: die with full stack trace because I really wasn't expecting that.

But I think considering the goals of the library the current behavior is fine.

One thing that troubled me (not enough to change it :) ) is that the message for missing required options includes all synonyms of the missing required options:

Here I have two options

  • "p" or "place"
  • "d" or "data-dir"

The message:
"Missing required option(s) ['d', 'p', 'place', 'data-dir']"

Not easy to see how to get around it through, without introducing some notion of the "preferred" form/name for an option.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Apr 6, 2014

Collaborator

@andrewbts I see what you're getting at. I wonder if changing the message of exception MissingRequiredOptionException (and other exceptions that have the context of all synonyms of an option) to indicate that these are synonyms would help? Maybe something like:

Missing required option (synonyms): ['d', 'p', 'place', 'data-dir']

This doesn't introduce any notion of a preferred synonym, but perhaps wouldn't give the impression that four required options are missing?

Collaborator

pholser commented Apr 6, 2014

@andrewbts I see what you're getting at. I wonder if changing the message of exception MissingRequiredOptionException (and other exceptions that have the context of all synonyms of an option) to indicate that these are synonyms would help? Maybe something like:

Missing required option (synonyms): ['d', 'p', 'place', 'data-dir']

This doesn't introduce any notion of a preferred synonym, but perhaps wouldn't give the impression that four required options are missing?

@andrewbts

This comment has been minimized.

Show comment
Hide comment
@andrewbts

andrewbts Apr 6, 2014

Interesting idea. I think I prefer the old message to this though, since the new one could be confusing to end-users of command line tools. If they are not familiar with jopt-simple, they may think the message implies something about a "synonym" option.

Any prospect for grouping the listed option names by synonymity?

Missing required option(s): ['d' or 'data-dir', 'p' or 'place']

andrewbts commented Apr 6, 2014

Interesting idea. I think I prefer the old message to this though, since the new one could be confusing to end-users of command line tools. If they are not familiar with jopt-simple, they may think the message implies something about a "synonym" option.

Any prospect for grouping the listed option names by synonymity?

Missing required option(s): ['d' or 'data-dir', 'p' or 'place']

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Apr 6, 2014

Collaborator

Definitely a possibility. I'll explore this.

Collaborator

pholser commented Apr 6, 2014

Definitely a possibility. I'll explore this.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Apr 6, 2014

Collaborator

Hi @andrewbts, I have a branch called 'exception-message-experiment' containing some changes to the exception messaging. Have a look, if you please, and see whether these changes suit you.

There are some API changes to support this -- mostly accepting lists of option specs rather than collections, renaming some exceptions (thank goodness they aren't part of the public API!), etc.

Bringing @binkley in on this also so he can weigh with his opinions.

Collaborator

pholser commented Apr 6, 2014

Hi @andrewbts, I have a branch called 'exception-message-experiment' containing some changes to the exception messaging. Have a look, if you please, and see whether these changes suit you.

There are some API changes to support this -- mostly accepting lists of option specs rather than collections, renaming some exceptions (thank goodness they aren't part of the public API!), etc.

Bringing @binkley in on this also so he can weigh with his opinions.

@andrewbts

This comment has been minimized.

Show comment
Hide comment
@andrewbts

andrewbts Apr 14, 2014

Hi, I don't think I'm going to get a chance to look at this any time soon sorry. Sudden project priority change at my end.

andrewbts commented Apr 14, 2014

Hi, I don't think I'm going to get a chance to look at this any time soon sorry. Sudden project priority change at my end.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Apr 14, 2014

Collaborator

OK, no worries. It'll be there when you're ready. 8^)

Collaborator

pholser commented Apr 14, 2014

OK, no worries. It'll be there when you're ready. 8^)

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Jul 30, 2014

Collaborator

@andrewbts @binkley Any further thoughts on this potential change? I'm inclined to bring it in unless I hear otherwise.

Collaborator

pholser commented Jul 30, 2014

@andrewbts @binkley Any further thoughts on this potential change? I'm inclined to bring it in unless I hear otherwise.

@EdJoJob

This comment has been minimized.

Show comment
Hide comment
@EdJoJob

EdJoJob Aug 8, 2014

Internationalization of the exception messages would be appreciated

EdJoJob commented Aug 8, 2014

Internationalization of the exception messages would be appreciated

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Aug 17, 2014

Collaborator

@EdJoJob I'm going to split out the i18n work into another (related) issue.

Collaborator

pholser commented Aug 17, 2014

@EdJoJob I'm going to split out the i18n work into another (related) issue.

pholser added a commit that referenced this issue Aug 17, 2014

For #58, redo exception messaging to clarify synonymous options.
Synonyms will be separated by slashes (/); lists of distinct options will
be separated by commas (,) in square brackets ([]).
@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Aug 18, 2014

Collaborator

@andrewbts @EdJoJob Latest 4.8 snapshot at oss.sonatype.org should have this. Let me know whether this satisfies your needs.

Collaborator

pholser commented Aug 18, 2014

@andrewbts @EdJoJob Latest 4.8 snapshot at oss.sonatype.org should have this. Let me know whether this satisfies your needs.

@pholser

This comment has been minimized.

Show comment
Hide comment
@pholser

pholser Sep 15, 2014

Collaborator

Closing -- both 4.x and master should have the exception message changes.

Collaborator

pholser commented Sep 15, 2014

Closing -- both 4.x and master should have the exception message changes.

@pholser pholser closed this Sep 15, 2014

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