mirage / irmin

Irmin is a distributed database that follows the same design principles as Git
https://irmin.org
ISC License
1.85k stars 157 forks source link

irmin-pack: initial integration of lower into store api #2188

Closed metanivek closed 1 year ago

metanivek commented 1 year ago

This PR does a few things to integrate the lower module into the higher level store code:

There are some TODOs for subsequent PRs:

(Note: this PR is based on #2184 so skip the first couple of commits if reviewing before it is merged)

codecov-commenter commented 1 year ago

Codecov Report

Merging #2188 (49adc03) into main (e68f6a7) will increase coverage by 0.04%. The diff coverage is 77.96%.

:exclamation: Current head 49adc03 differs from pull request most recent head 817dfd7. Consider uploading reports for the commit 817dfd7 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2188      +/-   ##
==========================================
+ Coverage   68.09%   68.13%   +0.04%     
==========================================
  Files         135      135              
  Lines       16222    16294      +72     
==========================================
+ Hits        11046    11102      +56     
- Misses       5176     5192      +16     
Impacted Files Coverage Δ
src/irmin-pack/unix/errors.ml 28.88% <ø> (ø)
src/irmin-pack/unix/io_errors.ml 58.33% <ø> (ø)
src/irmin-pack/version.ml 44.82% <20.00%> (+0.38%) :arrow_up:
src/irmin-pack/unix/store.ml 63.81% <50.00%> (+0.42%) :arrow_up:
src/irmin-pack/unix/file_manager.ml 82.89% <70.00%> (-1.23%) :arrow_down:
src/irmin-pack/unix/control_file.ml 91.52% <91.80%> (+1.20%) :arrow_up:
src/irmin-pack/unix/control_file_intf.ml 95.45% <100.00%> (-4.55%) :arrow_down:
src/irmin-fs/unix/irmin_fs_unix.ml 67.74% <0.00%> (-0.65%) :arrow_down:
src/irmin-test/store.ml 94.89% <0.00%> (-0.06%) :arrow_down:
... and 1 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

metanivek commented 1 year ago

@adatario just a few thoughts on errors...

This is the first time I'm adding several new errors to our existing setup. For now, I'm trying to follow prior conventions, but I too am wondering what the best approach is. I think there is some balance between specificity and general, between an error for each case and Msg of string :smile:, so welcome any and all feedback/thoughts!

To my knowledge, none of these errors leak outside of irmin-pack since we either raise them as exceptions (Errs.raise_if_error, Errs.raise_error) or map them to some generic Msg of string (in Gc). This means we have room to change them how we see fit since there little to no external "API" that could break by doing so.

Here are two guiding principles when I'm thinking about errors:

  1. Errors help us document meaningfully different error scenarios -- they are a communication tool for us as we work on the code
  2. Errors contain information to help pinpoint and debug an issue if it is printed by a client of irmin-pack (like octez) -- they are a communication tool for us as we debug issues
adatario commented 1 year ago

@adatario just a few thoughts on errors...

Thanks! That makes sense.