feat: Switch to color_eyre and remove unneeded .unwrap()s

http-server-rs / http-server

Simple and configurable command-line HTTP server

https://crates.io/crates/http-server

Apache License 2.0

176 stars 20 forks source link

feat: Switch to color_eyre and remove unneeded .unwrap()s #426

Open Antosser opened 6 months ago

Antosser commented 6 months ago

This PR is a lot. I've completely swapped anyhow with color_eyre, and replaced most unwraps with ?. Such changes required changing multiple function signatures and struct fields.

Now, the errors should look way better than they did before.

Examples

Before

After

Before

After

Also #425

Antosser commented 6 months ago

Hi @Antosser! Thanks so much for this PR. Can you please split concerns in two PRs please?

unwrap removals

color_eyre (this needs extra discussion)

Lets focus on merge 1 and then revisit 2 once 1 is merged!

color_erye is pretty much just a drop-in replacement for anyhow, but changing the imports over 25 files would still take some time. Why don't we discuss color_eyre here and maybe swap it for something else once rather than doing it several times?

EstebanBorai commented 6 months ago

Hi @Antosser! Thanks so much for this PR. Can you please split concerns in two PRs please?

unwrap removals

color_eyre (this needs extra discussion)

Lets focus on merge 1 and then revisit 2 once 1 is merged!

color_erye is pretty much just a drop-in replacement for anyhow, but changing the imports over 25 files would still take some time. Why don't we discuss color_eyre here and maybe swap it for something else once rather than doing it several times?

I rather focusing in each topic, makes PRs easier to review and also development in general easier to follow.

Given that we are writing this for the community, these kind of decisions needs some discussion on why we need to migrate.

Can we focus in unwraps for this PR, please?

Antosser commented 6 months ago

Can we focus in unwraps for this PR, please?

Rewriting everything right now would be a lot of work, just to probably undo it later. Let's discuss it first, so I won't have to do as many changes

Antosser commented 6 months ago

I started a discussion for this: #427

EstebanBorai commented 6 months ago

I started a discussion for this: #427

Nice! Let's just remove unwraps in this PR please, so we can merge it!

Antosser commented 6 months ago

@EstebanBorai

Let's just remove unwraps in this PR

Wdym?

EstebanBorai commented 6 months ago

@Antosser in this PR you add color_eyre which is out of the scope of this issue (#424).

https://github.com/http-server-rs/http-server/pull/426/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R60

Please revert that change so we focus on unwrap instances. Then, after merging, we can get back to color-eyre as alternative.

Antosser commented 6 months ago

As I've said, migrating back to anyhow is not as simple as find&replacing the Result structs, and it would take some time. Besides, I have no idea how to revert a commit from a PR in another PR, so it would be essentially double the work. I've also implemented some eyre-specific stuff, like suggestions, which would get lost in a migration back to anyhow. Let's discuss color_eyre first, before making huge changes

EstebanBorai commented 6 months ago

As I've said, migrating back to anyhow is not as simple as find&replacing the Result structs, and it would take some time. Besides, I have no idea how to revert a commit from a PR in another PR, so it would be essentially double the work. I've also implemented some eyre-specific stuff, like suggestions, which would get lost in a migration back to anyhow. Let's discuss color_eyre first, before making huge changes

I think I can help you. I will open a PR addressing #424. Then we merge and we can rebase against this. WDYT?

EstebanBorai commented 6 months ago

To be specific, in this PR we are addressing 2 concerns that are strongly mixed.

Each error message should be handled and must provide a clear path for the user to address the error/warning.

Reviewing this PR correctly its highly time consuming and that could easily be tackled by maintaining each PR scoped to a single concern.

Antosser commented 6 months ago

Then we merge and we can rebase against this. WDYT?

sure

Reviewing this PR correctly its highly time consuming

Yeah, it's a lot

that could easily be tackled by maintaining each PR scoped to a single concern

How? The amount of changes will stay the same. Only difference is that the error-handling library will be the same as before. Migrating back would take me much more time than for you to review this