search

haskell-nix / hnix-store

Haskell implementation of the Nix store
Apache License 2.0
87 stars 23 forks source link

Unify hash Base encoding/decoding funcitons #87

Closed Anton-Latukha closed 3 years ago

Anton-Latukha commented 3 years ago

Minimal start on refactoring the module interface step-by-step towards Cryptonite migration.

sorki commented 3 years ago

Looks reasonable - maybe we can drop encodeBaseXY altogether and use encodeIn, decode instead (encodeBase | decodeBase sounds even better).

There are only a few places that use *codeBase16 *codeBase32 outside of the module.

Can you please rebase on master? It needs adding qualified B32 in test.

sorki commented 3 years ago

After rebase you can cherry-pick https://github.com/sorki/hnix-store/commit/b26a8159bae4dc8924eca81a8202cf79de829cb5

sorki commented 3 years ago

I would prefer encodeBase | decodeBase naming, decode is rather ambiguous (e.g. used by Cereal and tons of other libs).

Anton-Latukha commented 3 years ago

I agree about encode/decode being too generous here.

The root cause for me is - encode/decode are general names, but suddenly they do only the Base encoding.

And in functions which purpose from the name (and from the type) is ambiguous - I like to indicate the direction. While decodeBase is grammatically correct - and encodeBase - "encode the base", or "encode into the base" - that is why using encodeIntoBase or more pidgin minimalistic encodeInBase - it creates minor inconvenience for the writer, but it gives ease and right understanding during reading, so tend to insert that to remove the ambiguity.

Anton-Latukha commented 3 years ago

This seems good enough.

Anton-Latukha commented 3 years ago

Thank you also.