Skip to content

Conversation

@hungfnt
Copy link
Contributor

@hungfnt hungfnt commented May 2, 2024

Refactor: HTTP tracker client returns errors instead of panicking.

@codecov
Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 78.90%. Comparing base (90c7780) to head (895efe9).

Files Patch % Lines
src/shared/bit_torrent/tracker/http/client/mod.rs 0.00% 15 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #826      +/-   ##
===========================================
- Coverage    78.92%   78.90%   -0.03%     
===========================================
  Files          163      163              
  Lines         9212     9215       +3     
===========================================
  Hits          7271     7271              
- Misses        1941     1944       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@josecelano josecelano left a comment

Choose a reason for hiding this comment

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

Hi @ngthhu, I'm going to merge this becuase this is better than what we have currently. However, It would be good to open a discussion to make your choice explicit and document it. You decided to use the anyhow crate. I'm not sure if in this case that's the best option.

We could use two alternatives:

  • Enums
  • Type-erased errors

Even anyhow recommends using thiserror crate "if you are a library that wants to design your own dedicated error type(s) so that on failures the caller gets exactly the information that you choose".

In this case, it seems clients might be interested in handling the different errors, but looking at another similar library like request they also used a BoxError. Maybe because there could be a lot of them and it would be hard to maintain an Enum.

Since this is not part of the public API (yet) it's not critical. If we decide to publish a crate we have to think about it carefully.

So it looks like a good choice to me.

To know more about "Enum" vs "Type-erased" errors I recommend the chapter 2 (ERROR HANDLING) of the Jon Gjengset' book:

https://rust-for-rustaceans.com/

@josecelano
Copy link
Member

ACK 895efe9

@josecelano josecelano merged commit f64e8fb into torrust:develop May 2, 2024
@josecelano
Copy link
Member

Thank your @ngthhu, and remember to sign commits 🙏🏼.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Cleanup / Refactoring Tidying and Making Neat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: bit_torrent::tracker::http::client::Client to return errors instead of panicking

2 participants