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

Add VerificationTreeDisplay #48

Merged
merged 1 commit into from
May 24, 2023
Merged

Conversation

nick-mobilecoin
Copy link
Collaborator

The VerificationTreeDisplay allows one to display the output of a
verification, showing which phases succeeded, which failed and what the
values were for each phase.

Motivation

Future Work

@github-actions github-actions bot added the size/XXL Double-wide PRs label Mar 19, 2023
verifier/src/lib.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

❗ No coverage uploaded for pull request base (nick/change-verifiction-output@a95d42c). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 48f4959 differs from pull request most recent head a83bf08. Consider uploading reports for the commit a83bf08 to get more accurate results

@@                        Coverage Diff                        @@
##             nick/change-verifiction-output      #48   +/-   ##
=================================================================
  Coverage                                  ?   97.10%           
=================================================================
  Files                                     ?        3           
  Lines                                     ?     1003           
  Branches                                  ?        0           
=================================================================
  Hits                                      ?      974           
  Misses                                    ?       29           
  Partials                                  ?        0           

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

Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

I thought we'd discussed that the primary source of the string at each print should be the display of the verifier...?

@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Mar 21, 2023

I thought we'd discussed that the primary source of the string at each print should be the display of the verifier...?

I think that's the self in this , https://github.com/mobilecoinfoundation/attestation/pull/48/files#diff-fe2f7ab2bdcef7981ced2b0119aa6146527d7beebe5bf3808e8652f6663e1eadR115

write!(f, "{:pad$}{status} {self}", "")?;

There are some places where that isn't done:

  • And It felt a little odd to have "the following" if And was displayed by itself, perhaps the message "Both of the following must be true" should be changed to "Both must be true".
  • Or similar to above
  • Not, AlwaysTrue, AlwaysFalse, I'll work on changing these done in 48f4959
  • AttributesVerifier similar to And, the separate line by line comparison doesn't seem to lend itself to the Verifier display well
  • MrSignerVerifier similar to the And/AttributesVerifier

@nick-mobilecoin
Copy link
Collaborator Author

my bad @cbeck88, I should have moved this to a draft, I broke it up and refactored in #86 and #87, with a follow on soon

@nick-mobilecoin nick-mobilecoin marked this pull request as draft May 11, 2023 19:38
@nick-mobilecoin nick-mobilecoin changed the base branch from main to nick/change-verifiction-output May 11, 2023 20:16
@nick-mobilecoin nick-mobilecoin marked this pull request as ready for review May 11, 2023 20:16
@github-actions github-actions bot added the size/XL Extra-Large PRs label May 11, 2023
@github-actions github-actions bot removed the size/XXL Double-wide PRs label May 11, 2023
@meowblecoinbot meowblecoinbot requested a review from a team May 11, 2023 20:17
@meowblecoinbot meowblecoinbot requested review from awygle and removed request for a team May 11, 2023 20:17
@github-actions
Copy link

❌ Unreviewed dependencies found

Crate Version Reviews (N/2) LoC Left-Pad Index Geiger Flags

@nick-mobilecoin nick-mobilecoin requested review from cbeck88 and removed request for awygle May 11, 2023 20:17
@nick-mobilecoin
Copy link
Collaborator Author

requested a re-review, @cbeck88, since this got refactored some with the new main line as well as #86 and #87

@cbeck88
Copy link

cbeck88 commented May 11, 2023

Np

The `VerificationTreeDisplay` allows one to display the output of a
verification, showing which phases succeeded, which failed and what the
values were for each phase.
Copy link

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

This looks good to me

@nick-mobilecoin nick-mobilecoin merged commit ce7f3d6 into main May 24, 2023
24 of 25 checks passed
@nick-mobilecoin nick-mobilecoin deleted the nick/displayable-error-tree branch May 24, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Extra-Large PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants