Remove .unwraps and .expects

[KoltesDigital]

Some idiomatic construct.

I was actually interested in displaying more human-friendly error messages (see throw) but saw you always use panic. I'm used to return anyhow::Result. Then, just replace all .unwrap() with ?. It not as human-friendly as a CLI should be, but at least it displays the error's source chain.

[chevdor]

but saw you always use panic

There is no good reason for it so your PR is welcome. I think color-eyre also provides a friendlier output when using proper error handling.

[chevdor]

Thanks a lot for this PR. Taking care of the error handling is definitey a good thing to do.

I would suggest to ensure then that it is complete by creating a config.toml under .cargo with content such as here so we enforce with clippy that unwrap are not used at all.

[chevdor]

Please don't force push unless absolutely required.

[KoltesDigital]

I disagree for force pushes. While a branch has not been merged to the trunk, it's common practice to update it and to change its history (aka "clean your branch"). For instance, say a reviewer finds a typo in a PR: the proposer just has to fix it, git absorb, and push force-with-lease. Of course, when it has been merged, changing its history is a no-go.

[KoltesDigital]

Ok there are no unwrap anymore.

I didn't read your message before about clippy. While it's actually possible to remove them all from the current code, as this PR does, I'm still puzzled about its ban. I think .unwrap could still be used in production, given that they assert things. Such errors are on the programming side, not on the usage side. However they must be rare, so this config is rather for teaching contributors.

That said, I failed to have Clippy warm against it. Neithor with .cargo/config.toml nor with .clippy.toml. error reading Clippy's configuration file 'XYZ\tera-cli\.clippy.toml': unknown field 'unwrap_used', expected one of .... It seems this key doesn't exist anymore, if it ever has.

[chevdor]

Did you use nightly ? cargo +nightly clippy...

[alerque]

Both the GitHub UI and CLI tooling has ways to compare changes between series of commits before and after a force push to make incremental PR review easy whether dealing with new commits or rewritten history.

[KoltesDigital]

@chevdor I wasn't aware of the +nightly flag, thanks. But that doesn't work either, there are still no config key dealing with unwrap. Well to be complete there's allow-unwrap-in-tests, which admittedly would mean that unwrap is banned from the rest of the code...

[chevdor]

There is a rule for unwrap. allow-unwrap-in-tests is unfortunately missing indeed. it should come with the next versions but this is not stable yet.

There are a few options:

• replace unwrap by expect but I really don't like this option
• do not run clippy on the tests at all
• spcificially exclude the unwrap rule from the tests

[chevdor]

which admittedly would mean that unwrap is banned from the rest of the code...

that would be for the better.
libs should not unwrap but return errors.
For the cli, this is different and what you did with color_eyre looks good to me, especially with the additional contextes you brought in.

Let me know once you are happy on your end with the state of this PR.
On my end, once we have "-Dclippy::unwrap_used" included in the quick-check to ensure that we don't regress from your cleanup, I will be ok.

[KoltesDigital]

Well .clippy.toml is for lint messages with configurable options, not for toggling lint messages themselves... I've put the switches at the top of main.rs and it worked.

Note that clap's parse wraps try_parse .expect but the error message is meant to be displayed as such (e.g. it includes hints), thus I let it so.

So I think I'm done!