rust-italia / dgc

A parser and validator for the EU Digital Green Certificate (dgc) a.k.a. greenpass
https://github.com/rust-italia/dgc
MIT License
26 stars 11 forks source link

Use match-return pattern #24

Closed allevo closed 2 years ago

allevo commented 2 years ago

In order to reduce "unwrap" usage, I'm proposing match return pattern. Reuse also decode_cwt internally.

codecov-commenter commented 2 years ago

Codecov Report

Merging #24 (e4af616) into main (5e4db0f) will decrease coverage by 0.18%. The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   82.36%   82.18%   -0.19%     
==========================================
  Files          10       10              
  Lines        1072     1061      -11     
==========================================
- Hits          883      872      -11     
  Misses        189      189              
Impacted Files Coverage Δ
src/parse.rs 71.65% <62.50%> (-2.26%) :arrow_down:

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 5e4db0f...e4af616. Read the comment docs.

lmammino commented 2 years ago

I am not extremely convinced by the fact that these errors are returned as Oks. If we refactor this piece shouldn't we definitely fix this? The code would become cleaner as well, because it is possible to use the try operator at that point.

@dodomorandi, the rationale behind that design is that you might still want to display some information about the content of a certificate, even if you don't manage to validate it.

Imagine, for instance, the case when you don't have the key in your trustlist. You are actually not even in a position to tell whether the certificate is valid or not and it might still be valuable to visualise all the info in the cert.

The current assumption is that, if it can decode the certificate correctly you are going to get an Ok and then certificate validity (for now only looking at the signature) is managed independently with that SignatureValidity enum...

I am more than happy to consider other ways to deal with this, but I'd like to be permissive with the validation and return all the data we can return in case of failure.

allevo commented 2 years ago

maybe can we pen a dedicated issue for discussing which design we would like to have (and why)?

lmammino commented 2 years ago

I think #21 might be a good issue to continue discussing the validation api. Meanwhile I'd propose we consider merging this if someone else thinks it helps to make the code a bit cleaner and more idiomatic!