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

Custom exception handler #74

Closed
vrobles-gee opened this issue Jul 24, 2020 · 2 comments
Closed

Custom exception handler #74

vrobles-gee opened this issue Jul 24, 2020 · 2 comments

Comments

@vrobles-gee
Copy link
Contributor

Hi

In recent days, I had and issue when one of my commands did throw an exception without managing it internally. It caused the TelnetServer thread to die, so not only the current session was lost, but we could not connect again until the application was restarted. We already added some fixes at application level, to make sure exceptions are properly catch on every of our commands, as well as some code to warranty a new TelnetServer is started in case the running one dies.

While doing all this, I was wondering if the library should allow the developer to specify a custom exception handler. The reason for this is not only to avoid relaying on each developer to properly manage the possible exceptions on each command, but also at least three more reasons:

  • Avoid duplication of code: right now I am doing the same thing for about 40 different commands, and even if I avoided code duplication by other means, it would be cleaner just have it as part of the library
  • Safety: The library could have a default handler that just print to the output stream the exception and can be overriden and/or enabled by the developer. That way even if developer does nothing, exceptions will never make the CLI to crash
  • Visibility: On my current solution, I have limited visibility since I am catching the exception inside the command lambda. For instance, I can print the exception on my logs, but I can't print the exact command the user typed to help me reproduce and fix the issue.

So basically I wanted to ask if there is anything already in place that I missed, or any reason to not include the feature. In case you agree with the idea, I am willing to implement that, but would like to ask your opinion about the best place to add that. I was thinking maybe catching the exception inside the Exec function, and just calling a handler registered by the developer on the Cli object if available, or printing an error to the session output stream otherwise.

Thanks

@daniele77
Copy link
Owner

Thank you very much, @vrobles-gee for pointing out that this feature is missing.
While a custom exception handler is a nice feature to have, a library exception handler is instead a must.

I already added both features in the repository. You can find them in the master. There is a unit test and a sample usage in the "complete" example application.

Please, let me know whether it suits your needs and whether the feature can be improved.

Thanks.

@vrobles-gee
Copy link
Contributor Author

Hi @daniele77 I just tested the new feature and it works as expected. I think this issue can be closed now. Thanks for such a quick reply and adding the new code in record time. I really enjoy collaborating in a project managed by someone so open to improvements like you, keep the good work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants