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

Friendly (traceless) error messages #77

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DannyBen
Copy link
Contributor

@DannyBen DannyBen commented Aug 5, 2016

This PR comes to improve error messages by showing only the message, and not the trace.

It rescues two exceptions:

  • ArgumentError - raised in some cases across the code, for example - when providing an invalid argument to --min-time
  • OptionParser::InvalidOption - which is raised when parsing unexpected option
  • Closes Friendly error messages #61

After merging, you get these:

$ papertrail --min-time asd
Argument Error: Could not parse time string 'asd'

$ papertrail --asd
invalid option: --asd

@lmarburger
Copy link
Contributor

Good addition. I'd be in favor of moving the exception handling behavior out into bin/papertrail in the same way interrupts are handled by adding another rescue clause for those 2 exceptions.

@DannyBen
Copy link
Contributor Author

DannyBen commented Aug 8, 2016

Makes sense. Will update the PR later today.

@DannyBen
Copy link
Contributor Author

DannyBen commented Aug 8, 2016

@lmarburger - better?

@DannyBen
Copy link
Contributor Author

DannyBen commented Aug 9, 2016

Note that in the other bin+lib pairs, there is also some inconsistency.

For example:

  1. in the add group lib file, there are rescue statements (which should probably go to the bin, as we did here).
  2. To achieve consistent behavior of the different CLI bins, all the bins should probably rescue the same baseline of exceptions (and this code duplication will reduce code quality metrics...).

@lmarburger lmarburger removed their assignment Dec 19, 2017
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

Successfully merging this pull request may close these issues.

2 participants