-
Notifications
You must be signed in to change notification settings - Fork 537
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
Convert NaiveDate::{from_yo|from_isoywd}
to return Result
#1460
Conversation
NaiveDate::{from_yo|from_isoywd} to return
Result`NaiveDate::{from_yo|from_isoywd}
to return Result
b3387d6
to
b151d68
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.5.x #1460 +/- ##
=======================================
Coverage 94.33% 94.33%
=======================================
Files 37 37
Lines 17004 17011 +7
=======================================
+ Hits 16040 16047 +7
Misses 964 964 ☔ View full report in Codecov by Sentry. |
b151d68
to
1ef8a85
Compare
Simplified |
src/naive/date/mod.rs
Outdated
if week == 0 || week > 53 { | ||
return Err(Error::InvalidArgument); | ||
} else { | ||
return Err(Error::DoesNotExist); | ||
} |
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, would suggest the following:
return Err(match week == 0 || week > 53 {
true => Error::InvalidArgument,
false => Error::DoesNotExist,
});
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.
For the next time just say 'Use match'. I should know by now 😆.
af0c228
to
d60fbaf
Compare
I'll keep this PR open a little longer until we have a new merge of main with the 0.5.x branch. |
d60fbaf
to
03727e8
Compare
cc @Zomtir. I would like to depend on these methods when working on the
Parsed
type, and you don't seem to have started on them yet.