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

Use match-return pattern #24

Merged
merged 1 commit into from
Nov 27, 2021
Merged

Use match-return pattern #24

merged 1 commit into from
Nov 27, 2021

Conversation

allevo
Copy link
Contributor

@allevo allevo commented Nov 25, 2021

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

Copy link
Member

@lmammino lmammino left a comment

Choose a reason for hiding this comment

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

I love this one! Thanks @allevo

Just one small note that this might generate some conflicts if work on #1 is already underway.

Copy link
Member

@lmammino lmammino left a comment

Choose a reason for hiding this comment

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

I love this one! Thanks @allevo

Just one small note that this might generate some conflicts if work on #1 is already underway.

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2021

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%) ⬇️

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.

Copy link
Contributor

@dodomorandi dodomorandi left a comment

Choose a reason for hiding this comment

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

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.

@lmammino
Copy link
Member

lmammino commented Nov 25, 2021

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
Copy link
Contributor Author

allevo commented Nov 25, 2021

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

@lmammino
Copy link
Member

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!

@lmammino lmammino merged commit 326f7cd into rust-italia:main Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants