-
Notifications
You must be signed in to change notification settings - Fork 547
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
Index Month
enum from 1
#1551
Index Month
enum from 1
#1551
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 0.5.x #1551 +/- ##
==========================================
+ Coverage 93.98% 94.01% +0.03%
==========================================
Files 37 37
Lines 16656 16579 -77
==========================================
- Hits 15654 15587 -67
+ Misses 1002 992 -10 ☔ View full report in Codecov by Sentry. |
@@ -42,29 +41,29 @@ use crate::OutOfRange; | |||
#[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(arbitrary::Arbitrary))] | |||
pub enum Month { | |||
/// January | |||
January = 0, | |||
January = 1, |
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.
Hmm, should we also make this explicitly repr(u8)
then?
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.
As I understand it the only extra thing repr(u8)
on an enum allows is unsafe pointer casting.
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.
Maybe, but to me it's also like a signal to the users about how this type works?
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.
With repr(u8)
it no longer has niche optimization, but I suppose there is little use for that.
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.
Okay, I would actually say that's a good reason not to add it.
/// ``` | ||
/// # use chrono::prelude::*; | ||
/// # use chrono::Month; | ||
/// let month = Month::January; | ||
/// let dt = Utc.at_ymd_and_hms(2019, month.number_from_month(), 28, 9, 10, 11).unwrap(); | ||
/// let dt = Utc.at_ymd_and_hms(2019, month as u32, 28, 9, 10, 11).unwrap(); |
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.
Should we change at_ymd_and_hms()
(and other places) to take u8
for month
?
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.
I don't know. A number of methods could take smaller arguments, such as NaiveTime::from_hms
. Is there an advantage for users in taking a smaller type?
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.
Smaller types would probably allow more optimization potential.
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.
I'll investigate and open an issue or PR. It does mean users have to do potentially fallible casts more often, so I'm not sure it is better for egonomics.
For 0.5 I'd like to make the
Month
enum less useless.By defining
Month::January
as 1 users can do simple casting, and use it for example in:We can also choose to make
Datelike::month
returnMonth
(#727). I didn't do so here for two reasons:year()
andday()
also return integers. I find working with integers more convenient than wordy month names.month
method onNaiveDate
that can't be optimized out without unsafe code. We knowMdf::from_ol(/* ... */).month()
will reaturn a value in1..=12
, but the compiler doesn't.