-
Notifications
You must be signed in to change notification settings - Fork 81
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
refactor maybe_known.rs: MaybeKnown enum #842
refactor maybe_known.rs: MaybeKnown enum #842
Conversation
nachiketkanore
commented
May 13, 2023
- fixes Move MaybeKnown enum to shared_vcx #839
Codecov Report
@@ Coverage Diff @@
## main #842 +/- ##
=======================================
Coverage 49.01% 49.01%
=======================================
Files 414 414
Lines 33694 33694
Branches 7445 7446 +1
=======================================
+ Hits 16515 16516 +1
Misses 12062 12062
+ Partials 5117 5116 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! There are just some small nitpicks that I commented on, but overall looks good. Thank you for picking this up!
@nachiketkanore Did you merge |
Yes, I merged |
Would you be so kind to rebase instead? |
Sorry for not following the guidelines. I am new to this, will check and update accordingly! UPD: I have rebased the main branch, let me know if there are any more issues. Thanks! |
Signed-off-by: Nachiket Kanore <nachiket.kanore@gmail.com>
Signed-off-by: Nachiket Kanore <nachiket.kanore@gmail.com>
Signed-off-by: Nachiket Kanore <nachiket.kanore@gmail.com>
9fba750
to
7dade1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, though I'd find it awesome if you also used some custom type for the Unknown
variant in the tests as well. But that can also be done sometime later and it's not really a dealbreaker.
Provided that the CI runs successfully, I'm okay with merging this.
It's all good, we all gotta start somewhere :). |