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

Exceptions hierarchy #11

Closed
den-mentiei opened this Issue Apr 1, 2013 · 13 comments

Comments

Projects
None yet
3 participants
@den-mentiei

den-mentiei commented Apr 1, 2013

In current version, ArgException is used for almost every exceptional situation - for args parsing errors, validating errors, and so on. Even such code, that's related to library using, uses ArgException:

var assembly = Assembly.GetEntryAssembly();
if (assembly == null)
{
    throw new ArgException("PowerArgs could not determine the name of your executable automatically.  This may happen if you run GetUsage<T>() from within unit tests.  Use GetUsageT>(string exeName) in unit tests to avoid this exception.");
}

The main issue that ArgException isn't useful, when it comes to proper error handling. For example:

     try
        {
            var parsed = Args.Parse<MyArgs>(args);
            Console.WriteLine("You entered string '{0}' and int '{1}'", parsed.StringArg, parsed.IntArg);
        }
        catch (ArgException ex)
        {
            Console.WriteLine(ex.Message);
            Console.WriteLine(ArgUsage.GetUsage<MyArgs>());
        }

Here we can't react properly - we just don't have information about exceptional situation - just a string message and that's all. We have 2 ways: to write message to the user (can be kinda cryptic for him) or to parse the string message (so we can know more about exceptional situation and react to it in other ways).

Solving this issue should involve exceptional situtions handling refactoring and creating of some exceptions hierarchy for the goods of library API.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 1, 2013

Owner

Hi,

My strategy has always been that any exception that is related to an error in the end user's input should result in an ArgException. That way, developers can have one catch block where it's always safe to show the message to the user and then print out the program's usage.

In other cases, the natural exception should flow through. The one example you show for the Assembly exception is a bug on my part. It should be changed to something Like an InvalidOperationException.

There are some cases where I intentionally catch some exceptions and then wrap them in an ArgException, but maybe you're suggesting that I don't do that for the user.

Can you please be more specific about the scenarios where you would expect a more specific exception?

Thanks,
Adam

Owner

adamabdelhamed commented Apr 1, 2013

Hi,

My strategy has always been that any exception that is related to an error in the end user's input should result in an ArgException. That way, developers can have one catch block where it's always safe to show the message to the user and then print out the program's usage.

In other cases, the natural exception should flow through. The one example you show for the Assembly exception is a bug on my part. It should be changed to something Like an InvalidOperationException.

There are some cases where I intentionally catch some exceptions and then wrap them in an ArgException, but maybe you're suggesting that I don't do that for the user.

Can you please be more specific about the scenarios where you would expect a more specific exception?

Thanks,
Adam

@den-mentiei

This comment has been minimized.

Show comment
Hide comment
@den-mentiei

den-mentiei Apr 2, 2013

Hello, Adam! I'll try to be more specific this time.

When end user's input is somehow wrong - I have ArgException. At this point my reaction can be:

  • just show exception's message to user
  • show some generic "everything is bad and you should feel bad" message

But if I want to have another complicated reaction - I don't have full information about occurred exception. For example, it's needed to show different internationalized messages according to the malformed end-user's input: one message for unexpected argument, another one for duplicated argument, etc.

Currently ArgException can be thrown when (mentioning just user input issues):

  • argument has been specified twice
  • wrong argument notation (/, -)
  • unexpected argument
  • wrong value for an argument
  • missing required argument
  • arguments' validation issues

And with instance of caught ArgException we can't know what've happened exactly.

I'm proposing to create an exception hierarchy - with different more specific exceptions derived from ArgException. E.g. UnexpectedArgException that has argument name property inside, etc. So, it should be possible to catch base one (react generally, or switch on type) or even have multiple catches per types.

ps. http://msdn.microsoft.com/en-us/library/vstudio/ms229064(v=vs.100).aspx

Consider providing exception properties for programmatic access to extra information (besides the message string) relevant to the exception.

den-mentiei commented Apr 2, 2013

Hello, Adam! I'll try to be more specific this time.

When end user's input is somehow wrong - I have ArgException. At this point my reaction can be:

  • just show exception's message to user
  • show some generic "everything is bad and you should feel bad" message

But if I want to have another complicated reaction - I don't have full information about occurred exception. For example, it's needed to show different internationalized messages according to the malformed end-user's input: one message for unexpected argument, another one for duplicated argument, etc.

Currently ArgException can be thrown when (mentioning just user input issues):

  • argument has been specified twice
  • wrong argument notation (/, -)
  • unexpected argument
  • wrong value for an argument
  • missing required argument
  • arguments' validation issues

And with instance of caught ArgException we can't know what've happened exactly.

I'm proposing to create an exception hierarchy - with different more specific exceptions derived from ArgException. E.g. UnexpectedArgException that has argument name property inside, etc. So, it should be possible to catch base one (react generally, or switch on type) or even have multiple catches per types.

ps. http://msdn.microsoft.com/en-us/library/vstudio/ms229064(v=vs.100).aspx

Consider providing exception properties for programmatic access to extra information (besides the message string) relevant to the exception.
@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 3, 2013

Owner

I think you make fair points. I like giving the dev the choice between showing the simple message or doing special handling if they choose the catch the specific exceptions. I'll consider this for an upcoming update (or feel free to take a stab at it, just let me know first so we don't both go do the same thing.

Thanks,
Adam

Owner

adamabdelhamed commented Apr 3, 2013

I think you make fair points. I like giving the dev the choice between showing the simple message or doing special handling if they choose the catch the specific exceptions. I'll consider this for an upcoming update (or feel free to take a stab at it, just let me know first so we don't both go do the same thing.

Thanks,
Adam

@den-mentiei

This comment has been minimized.

Show comment
Hide comment
@den-mentiei

den-mentiei Apr 3, 2013

I can work this out on upcoming weekend, wait for the pull request :)

den-mentiei commented Apr 3, 2013

I can work this out on upcoming weekend, wait for the pull request :)

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 4, 2013

Owner

Sweet! Thanks a lot.

Owner

adamabdelhamed commented Apr 4, 2013

Sweet! Thanks a lot.

@den-mentiei

This comment has been minimized.

Show comment
Hide comment
@den-mentiei

den-mentiei Apr 24, 2013

Sorry for being inactive, was kinda fully loaded with my work. I'll work this out asap! 😉

den-mentiei commented Apr 24, 2013

Sorry for being inactive, was kinda fully loaded with my work. I'll work this out asap! 😉

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Apr 25, 2013

Owner

No problem

Owner

adamabdelhamed commented Apr 25, 2013

No problem

@davidroberts63

This comment has been minimized.

Show comment
Hide comment
@davidroberts63

davidroberts63 May 22, 2013

Looks like it's been a little while. Anyone mind if I work on this? I'll wait a few days to hear back. If I don't read of any objections by March 25th I'll work on it on the 25 and 26th of March. Also any contribution guidelines?

davidroberts63 commented May 22, 2013

Looks like it's been a little while. Anyone mind if I work on this? I'll wait a few days to hear back. If I don't read of any objections by March 25th I'll work on it on the 25 and 26th of March. Also any contribution guidelines?

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed May 23, 2013

Owner

I assume you mean May 25, and that sounds reasonable. No real guidelines since there has not been much contribution. Maybe send a few sentences over regarding your strategy and which exceptions you plan to add. That way we can have some discussion before you spend time writing code.

Owner

adamabdelhamed commented May 23, 2013

I assume you mean May 25, and that sounds reasonable. No real guidelines since there has not been much contribution. Maybe send a few sentences over regarding your strategy and which exceptions you plan to add. That way we can have some discussion before you spend time writing code.

@davidroberts63

This comment has been minimized.

Show comment
Hide comment
@davidroberts63

davidroberts63 May 24, 2013

Yes I did mean May 25th (I seem to mix those two months up for some reason). As far as strategy. Just going through and review any thrown ArgExceptions to see if the message or situation are in multiple spots (valid values for example) or if they are significant situations. All of these would derive from ArgException to allow existing code to function normally. I don't believe adding any additional properties to these new exceptions would be needed.

First pass in about 10 minutes produced this list:

  • UnexpectedArgumentException: For both named and unamed arguments
  • DuplicateArgumentException: For "Argument specified more than once"
  • MissingArgumentException: For "The argument [argument] is required"
  • UnknownActionArgumentException: For "Unknown action"
  • MissingActionArgumentException: For "No action was specified" (This might just become 'MissingArgumentException')
  • QueryInvalidArgumentException: For "Could not compile your query"

I would recommend when "PowerArgs could not determine the name of your executable" that an InvalidOperationException be thrown instead of an ArgException. It's not a problem with a command line arg, it's a problem with the operational usage of the library at that moment, much like the message points out.

The ArgRevivers has a bit of inconsistency in using FormatException vs ArgException with regard to DateTime revival. How about just staying with FormatException since that seems to be the real problem anyway?

One that I am having a hard time pinning down in my mind is invalid argument values (ArgRange for example) and general argument validation problems (ArgExistingDirectory). At first I though of doing
* InvalidValueArgumentException for values that are 'well formed' but out of range (numbers, enums, length)
* ArgumentValidationException for any other general problem with the value (folder/file not found)
But since they both have 'Valid' in them I may just use the second one. Any suggestions would be appreciated.

This is just a first pass, if it feels like I'm going to far splitting out the exceptions just let me know.

davidroberts63 commented May 24, 2013

Yes I did mean May 25th (I seem to mix those two months up for some reason). As far as strategy. Just going through and review any thrown ArgExceptions to see if the message or situation are in multiple spots (valid values for example) or if they are significant situations. All of these would derive from ArgException to allow existing code to function normally. I don't believe adding any additional properties to these new exceptions would be needed.

First pass in about 10 minutes produced this list:

  • UnexpectedArgumentException: For both named and unamed arguments
  • DuplicateArgumentException: For "Argument specified more than once"
  • MissingArgumentException: For "The argument [argument] is required"
  • UnknownActionArgumentException: For "Unknown action"
  • MissingActionArgumentException: For "No action was specified" (This might just become 'MissingArgumentException')
  • QueryInvalidArgumentException: For "Could not compile your query"

I would recommend when "PowerArgs could not determine the name of your executable" that an InvalidOperationException be thrown instead of an ArgException. It's not a problem with a command line arg, it's a problem with the operational usage of the library at that moment, much like the message points out.

The ArgRevivers has a bit of inconsistency in using FormatException vs ArgException with regard to DateTime revival. How about just staying with FormatException since that seems to be the real problem anyway?

One that I am having a hard time pinning down in my mind is invalid argument values (ArgRange for example) and general argument validation problems (ArgExistingDirectory). At first I though of doing
* InvalidValueArgumentException for values that are 'well formed' but out of range (numbers, enums, length)
* ArgumentValidationException for any other general problem with the value (folder/file not found)
But since they both have 'Valid' in them I may just use the second one. Any suggestions would be appreciated.

This is just a first pass, if it feels like I'm going to far splitting out the exceptions just let me know.

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed May 24, 2013

Owner

Sounds good.


From: davidroberts63mailto:notifications@github.com
Sent: ‎5/‎24/‎2013 12:46 AM
To: adamabdelhamed/PowerArgsmailto:PowerArgs@noreply.github.com
Cc: Adam Abdelhamedmailto:adamabdelhamed@gmail.com
Subject: Re: [PowerArgs] Exceptions hierarchy (#11)

Yes I did mean May 25th (I seem to mix those two months up for some reason). As far as strategy. Just going through and review any thrown ArgExceptions to see if the message or situation are in multiple spots (valid values for example) or if they are significant situations. All of these would derive from ArgException to allow existing code to function normally. I don't believe adding any additional properties to these new exceptions would be needed.

First pass in about 10 minutes produced this list:

  • UnexpectedArgumentException: For both named and unamed arguments
  • DuplicateArgumentException: For "Argument specified more than once"
  • MissingArgumentException: For "The argument [argument] is required"
  • UnknownActionArgumentException: For "Unknown action"
  • MissingActionArgumentException: For "No action was specified" (This might just become 'MissingArgumentException')
  • QueryInvalidArgumentException: For "Could not compile your query"

I would recommend when "PowerArgs could not determine the name of your executable" that an InvalidOperationException be thrown instead of an ArgException. It's not a problem with a command line arg, it's a problem with the operational usage of the library at that moment, much like the message points out.

The ArgRevivers has a bit of inconsistency in using FormatException vs ArgException with regard to DateTime revival. How about just staying with FormatException since that seems to be the real problem anyway?

One that I am having a hard time pinning down in my mind is invalid argument values (ArgRange for example) and general argument validation problems (ArgExistingDirectory). At first I though of doing
* InvalidValueArgumentException for values that are 'well formed' but out of range (numbers, enums, length)
* ArgumentValidationException for any other general problem with the value (folder/file not found)
But since they both have 'Valid' in them I may just use the second one. Any suggestions would be appreciated.

This is just a first pass, if it feels like I'm going to far splitting out the exceptions just let me know.


Reply to this email directly or view it on GitHub:
#11 (comment)

Owner

adamabdelhamed commented May 24, 2013

Sounds good.


From: davidroberts63mailto:notifications@github.com
Sent: ‎5/‎24/‎2013 12:46 AM
To: adamabdelhamed/PowerArgsmailto:PowerArgs@noreply.github.com
Cc: Adam Abdelhamedmailto:adamabdelhamed@gmail.com
Subject: Re: [PowerArgs] Exceptions hierarchy (#11)

Yes I did mean May 25th (I seem to mix those two months up for some reason). As far as strategy. Just going through and review any thrown ArgExceptions to see if the message or situation are in multiple spots (valid values for example) or if they are significant situations. All of these would derive from ArgException to allow existing code to function normally. I don't believe adding any additional properties to these new exceptions would be needed.

First pass in about 10 minutes produced this list:

  • UnexpectedArgumentException: For both named and unamed arguments
  • DuplicateArgumentException: For "Argument specified more than once"
  • MissingArgumentException: For "The argument [argument] is required"
  • UnknownActionArgumentException: For "Unknown action"
  • MissingActionArgumentException: For "No action was specified" (This might just become 'MissingArgumentException')
  • QueryInvalidArgumentException: For "Could not compile your query"

I would recommend when "PowerArgs could not determine the name of your executable" that an InvalidOperationException be thrown instead of an ArgException. It's not a problem with a command line arg, it's a problem with the operational usage of the library at that moment, much like the message points out.

The ArgRevivers has a bit of inconsistency in using FormatException vs ArgException with regard to DateTime revival. How about just staying with FormatException since that seems to be the real problem anyway?

One that I am having a hard time pinning down in my mind is invalid argument values (ArgRange for example) and general argument validation problems (ArgExistingDirectory). At first I though of doing
* InvalidValueArgumentException for values that are 'well formed' but out of range (numbers, enums, length)
* ArgumentValidationException for any other general problem with the value (folder/file not found)
But since they both have 'Valid' in them I may just use the second one. Any suggestions would be appreciated.

This is just a first pass, if it feels like I'm going to far splitting out the exceptions just let me know.


Reply to this email directly or view it on GitHub:
#11 (comment)

@davidroberts63

This comment has been minimized.

Show comment
Hide comment
@davidroberts63

davidroberts63 Jun 5, 2013

Thanks for being good to work with. FYI I have written a blog post about this contribution (my attempt at blogging more often) at http://davidroberts63.bitbucket.org/blog/2013/05/28/fork-and-pull-powerargs/

davidroberts63 commented Jun 5, 2013

Thanks for being good to work with. FYI I have written a blog post about this contribution (my attempt at blogging more often) at http://davidroberts63.bitbucket.org/blog/2013/05/28/fork-and-pull-powerargs/

@adamabdelhamed

This comment has been minimized.

Show comment
Hide comment
@adamabdelhamed

adamabdelhamed Jun 8, 2013

Owner

Cool blog post. Sorry, I already fixed that multiple shortcuts issue so you won't be able to take that one. But feel free to suggest other features, improvements, bugs, etc.

Owner

adamabdelhamed commented Jun 8, 2013

Cool blog post. Sorry, I already fixed that multiple shortcuts issue so you won't be able to take that one. But feel free to suggest other features, improvements, bugs, etc.

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