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

Better panic handling #9

Open
mattgodbolt opened this issue Feb 4, 2022 · 5 comments
Open

Better panic handling #9

mattgodbolt opened this issue Feb 4, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@mattgodbolt
Copy link
Contributor

I'm not sure what the rust-y way to do this is but when I typo a timezone, this seems pretty nasty:

$ rizzy --zone not_a_tz
thread 'main' panicked at 'Could not parse timezone: not_a_tz, Error: 'not_a_tz' is not a valid timezone', src/main.rs:45:19
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@siedentop
Copy link
Contributor

Hi @mattgodbolt ,

I hacked together a solution with autocomplete. What do you think of the following output?

chris@christophs-air rizzy % cargo run -- -z America/Chi
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/rizzy -z America/Chi`
Error: 
   0: Unknown timezone 'America/Chi', did you mean: ["America/Chihuahua", "America/Chicago"]

Location:
   src/main.rs:103

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
  1. Demo draft is here, but would require for Make TIMEZONES map public, so that users may iterate over all timezones. chronotope/chrono-tz#95 to be merged (in some form or another). Timezones: Build auto-suggestion from all known timezones. #12
  2. My PR (Timezones: Build auto-suggestion from all known timezones. #12) only does the auto-suggestion and removes one panic!, but there's a few others that could be removed.

@mattgodbolt
Copy link
Contributor Author

Thanks @siedentop !

I was wondering about maybe not panic-ing at all in these cases and handling it more user-friendlyly. This is a huge improvement of course, but as a user of a tool, I probably never want to see anything specific to the language a command-line tool was written in if I mistyped an option :-)

I love the autocomplete thing, and I guess I would imagine an ideal output is:

% cargo run -- -z America/Chi
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/rizzy -z America/Chi`
Unknown timezone 'America/Chi', did you mean: ["America/Chihuahua", "America/Chicago"]
%

The rest is visual noise to the user (though handy if you are a developer). My sort of mental model is if you typed a bad command-line parameter to cat you get:

$ cat -f
cat: invalid option -- 'f'
Try 'cat --help' for more information.

and not something like

 cat -f
Error:
  0: invalid option -- 'f'

Location:
    src/cat.c:34

Backtrace omitted.
Run with DEBUG_.BINUTILS=1 environment variable to display it.
Run with DEBUG_BINUTILS=full to include source snippets.

(etc!)

I have this beef with plenty of Python tools that expose their innards to unsuspecting users if the user makes a mistake :)

@siedentop
Copy link
Contributor

Yes, totally!

The answer to that is to use eprintln!("Rizzy failed on: {}", error);.

I'll add a patch in a day or so.

@siedentop
Copy link
Contributor

I updated this. But the upstream library still has not had a response to my request to make the list of timezone names readable from the library.

@siedentop
Copy link
Contributor

siedentop commented May 20, 2022

UPDATE: It looks like the upstream library (chrono-tz) is willing to merge my patch.

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

No branches or pull requests

2 participants