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

[bugfix] Straighten out command-line error handling #106

Conversation

fako1024
Copy link
Collaborator

@fako1024 fako1024 commented Apr 11, 2023

What I've done:

  • Replaced all manual error handling / output in the cobra entrypoint function to simply return the error to the caller
  • Handle any error from that function in Execute(), using "our" logger with its Fatalf() method
  • Initialize our logger directly in init() instead of via cobra (to ensure it's already initialized if the error occurs before cobra can call the initializer)

@els0r For now I've kept this a draft because there are two open questions IMHO:

  • Should we maybe add a special formatter for the command line tool(s)? Right now we throw a message like this:
ts=2023-04-11T11:23:51.976+02:00 level=fatal msg="Error running query: failed to prepare query: failed to parse query type: Unknown attribute name: 'protod'" version=872abf81

TBH I don't think that someone running a CLI expects a "logger" style response but rather something like:

Error running query: failed to prepare query: failed to parse query type: Unknown attribute name: 'protod'
  • The same changes should probably be applied to global-query (because the same problem about not notifying errors happens there as indicated in global-query #43 )

What's your take on those open questions?

Closes #104

@fako1024 fako1024 added the bug Something isn't working label Apr 11, 2023
@fako1024 fako1024 added this to the v4 Release milestone Apr 11, 2023
@fako1024 fako1024 requested a review from els0r April 11, 2023 09:33
@fako1024 fako1024 self-assigned this Apr 11, 2023
@fako1024 fako1024 linked an issue Apr 11, 2023 that may be closed by this pull request
3 tasks
@els0r
Copy link
Owner

els0r commented Apr 11, 2023

Thanks for looking into this. As for your question about the custom formatter: it absolutely makes sense, but only where we log an error that is propagated from the entrypoint (e.g. where we previously did fmt.Frpintf(os.Stderr, ...).

Writing that slog.Handler should be trivial, or even fun. Which does bring me to another point though: I am almost certain that we need to split up initialization of the logging library and instantiation. In the sense that we can do:

logging.New(level, encoding, ...opts) and get a new *logging.L object with all the pretty handlers attached. And of course NewFromContext. The function signature is the same as for Init because you want to retain maximum control over what the logger does.

The Init function will then be nothing but a wrapper around New.

This would allow us to create a logger for only the CLI stuff, while the rest of the logging functionality is untouched.

@els0r
Copy link
Owner

els0r commented Apr 11, 2023

@fako1024 : see latest PR on logging. Sets the ground work for writing and using the PlainHandler only inside the main.go function of the respective tools.

Copy link
Owner

@els0r els0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested because we need the:

  • plain/CLI logging handler (I prefer plain because it's closer to what the logger does or doesn't do)
  • use the logger only in Execute
  • if err stuff because it'll create merge conflicts with global-query #43 that are tedious to fix

cmd/goQuery/commands/root.go Show resolved Hide resolved
cmd/goQuery/commands/root.go Outdated Show resolved Hide resolved
cmd/goQuery/commands/root.go Show resolved Hide resolved
cmd/goQuery/commands/root.go Show resolved Hide resolved
cmd/goQuery/commands/root.go Show resolved Hide resolved
@els0r els0r force-pushed the 104-command-line-error-handling-is-incoherent-and-misses-certain-issues branch from 6663649 to 7fb1420 Compare April 13, 2023 15:57
@els0r els0r force-pushed the 104-command-line-error-handling-is-incoherent-and-misses-certain-issues branch from 7fb1420 to d10a872 Compare April 14, 2023 20:24
Copy link
Owner

@els0r els0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go from my end. Feel free to merge when available.

cmd/goQuery/commands/root.go Show resolved Hide resolved
@fako1024 fako1024 marked this pull request as ready for review April 15, 2023 10:19
@fako1024
Copy link
Collaborator Author

Good to go from my end. Feel free to merge when available.

Awesome, thanks a lot for taking care of the CLI stuff! I just gave it a quick spin, seems to cover all cases now and error "results" look very readable now. 💪

@fako1024 fako1024 merged commit e77e667 into develop Apr 15, 2023
@fako1024 fako1024 deleted the 104-command-line-error-handling-is-incoherent-and-misses-certain-issues branch April 15, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command-line error handling is incoherent (and misses certain issues)
2 participants