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

Transition to std::error::Error; minor cleanup #28

Merged
merged 20 commits into from
May 14, 2022
Merged

Conversation

jssblck
Copy link
Contributor

@jssblck jssblck commented Apr 19, 2022

Closes #8
Closes #27

This is a breaking change.

Removes failure, and in its place implements errors as standard enums compatible with std::error::Error, powered by thiserror.
Also includes some minor cleanup and reorganization:

  • The top-level crate now exposes faktory::error::ProtocolError, which is the old FaktoryError type, and faktory::error::Error, which is all possible errors returned by the crate (one of which is faktory::error::ProtocolError).
  • Errors under producer and consumer now return the top-level faktory::error::Error type (this solves Use error type instead of using catch all failure::Error in Producer::con #8).
  • I also made Job.failure into pub, instead of pub(crate); I did this because clippy was complaining that it's unused. If you disagree happy to change it back.
  • I made a couple other minor edits based on clippy recommendations.

This change is Reviewable

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #28 (9cbbced) into master (a3524c8) will decrease coverage by 2.15%.
The diff coverage is 64.81%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   85.09%   82.93%   -2.16%     
==========================================
  Files          12       12              
  Lines         966      932      -34     
==========================================
- Hits          822      773      -49     
- Misses        144      159      +15     
Impacted Files Coverage Δ
src/consumer/mod.rs 69.04% <0.00%> (-0.84%) ⬇️
src/error.rs 63.63% <0.00%> (+9.09%) ⬆️
src/producer/mod.rs 76.19% <ø> (-4.77%) ⬇️
src/proto/single/mod.rs 80.00% <0.00%> (-5.72%) ⬇️
src/tls.rs 0.00% <0.00%> (ø)
tests/consumer.rs 100.00% <ø> (ø)
tests/mock/mod.rs 100.00% <ø> (+6.81%) ⬆️
tests/real.rs 100.00% <ø> (ø)
src/proto/mod.rs 68.14% <16.66%> (-1.39%) ⬇️
src/proto/single/resp.rs 80.27% <58.82%> (-14.38%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3524c8...9cbbced. Read the comment docs.

@jssblck
Copy link
Contributor Author

jssblck commented Apr 19, 2022

The codecov regression is due to the fact that we now have more explicit errors instead of sticking them in a catch-all type; I don't think this is an actual coverage regression. For that reason I'd like to request we overlook this, but if you disagree let me know and I can look for ways to explicitly test the new explicit error cases.

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! These are a great set of changes. I've left some mostly minor notes inline. And yes, happy to overlook the coverage change.

src/error.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Show resolved Hide resolved
src/proto/single/cmd.rs Outdated Show resolved Hide resolved
src/proto/single/mod.rs Outdated Show resolved Hide resolved
src/proto/single/resp.rs Outdated Show resolved Hide resolved
@jssblck
Copy link
Contributor Author

jssblck commented Apr 24, 2022

@jonhoo thanks for the great review! I think I addressed everything 😄

@jssblck jssblck requested a review from jonhoo April 24, 2022 18:33
Comment on lines +160 to +163
/// Data about this job's most recent failure.
pub fn failure(&self) -> &Option<Failure> {
&self.failure
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the minimal set of changes I wanted to include in this PR, and only did when forced to by clippy, so Failure is still unusable. I'm happy to commit to coming back in another PR to make Failure useful, or put it into this one if preferred.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's do that in a separate PR then, thanks 👍

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

We're very nearly there!

src/proto/mod.rs Outdated Show resolved Hide resolved
Comment on lines +160 to +163
/// Data about this job's most recent failure.
pub fn failure(&self) -> &Option<Failure> {
&self.failure
}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's do that in a separate PR then, thanks 👍

src/proto/single/resp.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/proto/single/cmd.rs Outdated Show resolved Hide resolved
@jssblck jssblck requested a review from jonhoo May 1, 2022 15:12
src/proto/single/resp.rs Outdated Show resolved Hide resolved
src/proto/single/cmd.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Just a few minor nits left and then we're there 🎉

@jssblck jssblck requested a review from jonhoo May 10, 2022 21:29
@jonhoo
Copy link
Owner

jonhoo commented May 14, 2022

Thanks for sticking with this! I'll release it as a .beta initially, and then in a week or two we can promote to a full release — how does that sound?

@jonhoo jonhoo merged commit 25883da into jonhoo:master May 14, 2022
@jonhoo
Copy link
Owner

jonhoo commented May 14, 2022

Released as 0.12.0-beta.1 🎉

@jssblck
Copy link
Contributor Author

jssblck commented May 17, 2022

Thanks for sticking with this! I'll release it as a .beta initially, and then in a week or two we can promote to a full release — how does that sound?

Sounds great!

@jonhoo
Copy link
Owner

jonhoo commented May 28, 2022

Published 0.12.0-beta.1 as 0.12.0!

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