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

Refactors digest-related code in src/lock.rs #151

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Conversation

xfbs
Copy link
Contributor

@xfbs xfbs commented Oct 25, 2023

This moves the digest-related code out of src/lock.rs and adds some tests for it. This increases overall test coverage (line coverage) by about 9%.

Notes:

  • We currently manually implement Serialize and Deserialize, this is usually not a good idea. It is better (less code) to use something like serde_with to implement it.
  • There is probably still a couple low hanging fruit but this PR tests the most important stuff.

Before:

Screenshot 2023-10-25 at 14 57 11

After:

Screenshot 2023-10-25 at 14 56 20

This moves the digest-related code out of `src/lock.rs` and writes tests
for it.
@xfbs xfbs added complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli type::refactoring Changing the inner-workings of buffrs priority::low Please dont work on this if you can contribute something with a higher priority labels Oct 25, 2023
@xfbs xfbs self-assigned this Oct 25, 2023
@xfbs xfbs requested review from asmello and mara-schulke October 25, 2023 13:00
@mara-schulke mara-schulke added this to the Stabilization milestone Oct 26, 2023
@mara-schulke mara-schulke merged commit abcde8b into main Oct 26, 2023
6 checks passed
@mara-schulke mara-schulke deleted the pe/refactor-lock branch October 26, 2023 19:40
}
}

#[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'm late to review, but I don't think this is good practice @xfbs. First time I've seen tests outside of a module gated by #[cfg(test)] and after reviewing the docs I'm pretty sure there's a good reason for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli priority::low Please dont work on this if you can contribute something with a higher priority type::refactoring Changing the inner-workings of buffrs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants