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

Replace failure crate with thiserror #684

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented Dec 2, 2022

Replaces the dependency on outdated failure crate with the dependency on thiserror in messages, aries-vcx-agent, aries-vcx and libvcx. Although our entire approach to logging needs to be reconsidered, removing a dependency on an unmaintained crate is likely not a step in the wrong direction.

It seems that we have to temporarily give up backtrace in libvcx errors stored in CURRENT_ERROR_C_JSON until error_generic_member_access is stabilized since thiserror makes use of unstable provide() method to the error type in order to capture and share std::backtrace::Backtrace.

@mirgee mirgee added refactoring agents dependencies Pull requests that update a dependency file labels Dec 2, 2022
@mirgee mirgee requested a review from a team as a code owner December 2, 2022 21:57
@mirgee mirgee force-pushed the refactor/remove-failure-dependency branch from bbad320 to 9c53ceb Compare December 2, 2022 22:18
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Merging #684 (619c697) into main (aa4aed2) will increase coverage by 19.81%.
The diff coverage is 16.66%.

@@             Coverage Diff             @@
##             main     #684       +/-   ##
===========================================
+ Coverage   42.66%   62.48%   +19.81%     
===========================================
  Files         234      237        +3     
  Lines       17824    23134     +5310     
  Branches     3521     5276     +1755     
===========================================
+ Hits         7605    14455     +6850     
+ Misses       7905     4551     -3354     
- Partials     2314     4128     +1814     
Flag Coverage Δ
unittests-aries-vcx 62.34% <16.66%> (+19.68%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aries_vcx/src/common/ledger/transactions.rs 51.89% <0.00%> (+35.23%) ⬆️
aries_vcx/src/indy/ledger/transactions.rs 50.00% <0.00%> (+40.65%) ⬆️
aries_vcx/src/utils/serialization.rs 0.00% <0.00%> (ø)
agency_client/src/error.rs 18.42% <5.00%> (+8.66%) ⬆️
aries_vcx/src/error.rs 14.57% <22.72%> (+1.35%) ⬆️
messages/src/error.rs 5.00% <23.07%> (-1.09%) ⬇️
aries_vcx/src/indy/signing.rs 60.34% <100.00%> (ø)
aries_vcx/tests/test_connection.rs 73.23% <0.00%> (-26.77%) ⬇️
aries_vcx/tests/test_pool.rs 76.82% <0.00%> (-23.18%) ⬇️
aries_vcx/tests/test_agency.rs 80.90% <0.00%> (-19.10%) ⬇️
... and 149 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mirgee mirgee force-pushed the refactor/remove-failure-dependency branch 2 times, most recently from e66bd33 to f72704a Compare December 3, 2022 20:38
@Patrik-Stas
Copy link
Contributor

Patrik-Stas commented Dec 3, 2022

@mirgee awesome! But I please rebase on #648 rather soon then later ;-) It's going to be the next going to be the next merge to main followed by 0.50.0 release right after

Patrik-Stas
Patrik-Stas previously approved these changes Dec 5, 2022
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

It's also great that indy-vdr and indy-shared-rs are both using thiserror

We'll just need rebase this

Signed-off-by: Miroslav Kovar <[email protected]>
@mirgee mirgee force-pushed the refactor/remove-failure-dependency branch from 32956f7 to 619c697 Compare December 5, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agents dependencies Pull requests that update a dependency file refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants