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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Antosser
Copy link
Contributor

@Antosser Antosser commented Mar 2, 2024

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

image

After

image

Before

image

After

image

Also #425

image

Copy link
Member

@EstebanBorai EstebanBorai left a comment

Choose a reason for hiding this comment

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

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

  1. unwrap removals
  2. color_eyre (this needs extra discussion)

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

@Antosser
Copy link
Contributor Author

Antosser commented Mar 3, 2024

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

  1. unwrap removals
  2. 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
Copy link
Member

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

  1. unwrap removals
  1. 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
Copy link
Contributor Author

Antosser commented Mar 3, 2024

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
Copy link
Contributor Author

Antosser commented Mar 3, 2024

I started a discussion for this: #427

@EstebanBorai
Copy link
Member

I started a discussion for this: #427

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

@Antosser
Copy link
Contributor Author

Antosser commented Mar 3, 2024

@EstebanBorai

Let's just remove unwraps in this PR

Wdym?

@EstebanBorai
Copy link
Member

@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
Copy link
Contributor Author

Antosser commented Mar 3, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

Antosser commented Mar 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants