-
Notifications
You must be signed in to change notification settings - Fork 166
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
Implement TryFrom for a variety of types #139
Conversation
src/time.rs
Outdated
@@ -47,9 +44,7 @@ impl Time { | |||
/// ``` | |||
#[cfg(feature = "std")] | |||
pub fn try_from(time: std::time::SystemTime) -> Result<Time, ring::error::Unspecified> { |
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.
Go ahead and just remove this function.
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.
Sounds good; I kept it just in case needing to add use std::convert::TryFrom
would be considered a breaking change
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
==========================================
- Coverage 73.91% 73.76% -0.15%
==========================================
Files 14 14
Lines 1357 1361 +4
==========================================
+ Hits 1003 1004 +1
- Misses 354 357 +3
Continue to review full report at Codecov.
|
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.
Thanks! I'm glad this cleanup is being done. Just a few small details to clean up and then I'll merge this.
src/webpki.rs
Outdated
/// `cert_der`. | ||
pub fn from(cert_der: &'a [u8]) -> Result<Self, Error> { | ||
EndEntityCert::try_from(cert_der) | ||
} |
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.
Sorry I wasn't clear in the earlier review. We should also delete this alternative so that TryFrom
is the only way to construct a EndEntityCert
.
tests/integration.rs
Outdated
@@ -87,5 +87,6 @@ fn read_root_with_neg_serial() { | |||
#[cfg(feature = "std")] | |||
#[test] | |||
fn time_constructor() { | |||
use std::convert::TryFrom; |
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.
Again. let's use core::
instead of std::
consistently.
src/webpki.rs
Outdated
@@ -108,17 +109,27 @@ pub struct EndEntityCert<'a> { | |||
inner: cert::Cert<'a>, | |||
} | |||
|
|||
impl<'a> EndEntityCert<'a> { | |||
impl<'a> TryFrom<&'a [u8]> for EndEntityCert<'a> { |
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.
NIT: In the other implementation you used impl ... core::convert::TryFrom
and here you used use ::cure::convert::TryFrom
and then the impl
is unqualified. I think we should be consistent.
This adds TryFrom implementations for a variety of types that already had equivalent implementations, but outside of an
imply TryFrom
.I'm unsure about adding
#[deprecated]
, since those who elevate deprecation warning to compile error may have touse core::convert::TryFrom
.related to #73