Skip to content
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

Merged
merged 3 commits into from
Apr 4, 2024
Merged

Index Month enum from 1 #1551

merged 3 commits into from
Apr 4, 2024

Conversation

pitdicker
Copy link
Collaborator

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:

let date = NaiveDate::from_ymd(2015, Month::March as u32, 14);

We can also choose to make Datelike::month return Month (#727). I didn't do so here for two reasons:

  • For my personal projects using integers for months was perfectly fine and matches that year() and day() also return integers. I find working with integers more convenient than wordy month names.
  • Returning en enum will introduce extra checks in the month method on NaiveDate that can't be optimized out without unsafe code. We know Mdf::from_ol(/* ... */).month() will reaturn a value in 1..=12, but the compiler doesn't.

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.01%. Comparing base (08c9792) to head (bb658ae).
Report is 5 commits behind head on 0.5.x.

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.
📢 Have feedback on the report? Share it here.

@@ -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,
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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();
Copy link
Contributor

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@pitdicker pitdicker merged commit 33c372c into chronotope:0.5.x Apr 4, 2024
35 checks passed
@pitdicker pitdicker deleted the month_enum branch April 4, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants