-
Notifications
You must be signed in to change notification settings - Fork 543
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
Checked NaiveWeek
methods
#1600
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1600 +/- ##
==========================================
- Coverage 91.12% 91.11% -0.02%
==========================================
Files 37 37
Lines 17057 17104 +47
==========================================
+ Hits 15543 15584 +41
- Misses 1514 1520 +6 ☔ View full report in Codecov by Sentry. |
This makes sense to me, thanks. In its current form the diffs get a little ugly, would you mind doing this with one commit per method and putting the panicking method above the |
a88f49a
to
ec07b3d
Compare
48164f8
to
7904578
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 modulo one nit, thanks! (Please fix it in the originating commit.)
7904578
to
473f7c2
Compare
Currently, all
NaiveWeek
's public methods panic on overflow. This PR allows to handle such scenarios.Seems to solve #1469 (
NaiveWeek
part).Result
makes sense here, since only overflow results inNone
. ThusOption
.checked_*
methods in order to not break API