Skip to content

Make methods on NaiveDateTime const where possible #1286

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.

Sign up for GitHub

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

Jump to bottom
Merged
pitdicker merged 2 commits into from
Sep 29, 2023
Merged

Make methods on NaiveDateTime const where possible #1286

pitdicker merged 2 commits into from
Sep 29, 2023

Conversation

@pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Sep 12, 2023

I had to copy a few methods from the Datelike and Timelike traits to const methods on NaiveDate and NaiveTime, to make use of them in const methods on NaiveDateTime.

Otherwise the changes are fairly straightforward.

Only DateTime<Tz> to go after this (and a handful of methods that depend on Duration).

@codecov
Copy link

codecov bot commented Sep 12, 2023 •
edited
Loading

Codecov Report

Merging #1286 (6b3a314) into 0.4.x (c8e3eb4) will increase coverage by 0.01%.
The diff coverage is 96.49%.

@@            Coverage Diff             @@
##            0.4.x    #1286      +/-   ##
==========================================
+ Coverage   91.53%   91.55%   +0.01%     
==========================================
  Files          38       38              
  Lines       17325    17352      +27     
==========================================
+ Hits        15859    15886      +27     
  Misses       1466     1466
Files Coverage Δ
src/datetime/mod.rs 89.34% <100.00%> (+0.04%) ⬆️
src/naive/date.rs 96.25% <100.00%> (+0.03%) ⬆️
src/naive/time/mod.rs 96.55% <100.00%> (+0.02%) ⬆️
src/naive/datetime/mod.rs 97.64% <93.75%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker
Copy link
Collaborator Author

pitdicker commented Sep 13, 2023

The CI pointed out an issue because of our MSRV:

error[E0658]: trait bounds other than `Sized` on const fn parameters are unstable
   --> src\datetime\mod.rs:98:6
    |
98  | impl<Tz: TimeZone> DateTime<Tz> {
    |      ^^
...
123 |     pub const fn from_naive_utc_and_offset(datetime: NaiveDateTime, offset: Tz::Offset) -> DateTime<Tz> {
    |     --------------------------------------------------------------------------------------------------- function declared as const here
    |
    = note: see issue #93706 <https://github.com/rust-lang/rust/issues/93706> for more information

For more information about this error, try `rustc --explain E0658`.
error: could not compile `chrono` due to previous error

I have added a workaround so that we can have at least one constructor for a DateTime: NaiveDateTime::and_utc.

@pitdicker pitdicker mentioned this pull request Sep 13, 2023
Open
@pitdicker pitdicker force-pushed the const_naivedatetime branch 2 times, most recently from 955db54 to ba8604c Compare September 27, 2023 13:02
@pitdicker pitdicker mentioned this pull request Sep 27, 2023
Open
jtmoon79
jtmoon79 approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

src/naive/date.rs
self.of().weekday()
}

/// Counts the days in the proleptic Gregorian calendar, with January 1, Year 1 (CE) as day 1.
Copy link
Contributor

@jtmoon79 jtmoon79 Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a fun recommendation: link to the meaning of "AD 1":

 /// ... with January 1, Year 1 (CE) as day 1, i.e. [AD 1](https://en.wikipedia.org/wiki/AD_1)

djc
djc approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the first three commits into the fourth one: they add code that goes unused.

@pitdicker
(Used in NaiveDate::timestamp*)
6cafed5 
Make methods on `NaiveDateTime` const where possible
@pitdicker
Make NaiveDateTime::and_utc const
6b3a314
@pitdicker pitdicker merged commit fe4713a into chronotope:0.4.x Sep 29, 2023
@pitdicker pitdicker deleted the const_naivedatetime branch September 29, 2023 09:14
This was referenced Jan 24, 2024
Closed
Closed
This was referenced Jan 28, 2024
Merged
Closed
@pitdicker pitdicker mentioned this pull request Feb 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djc djc djc approved these changes

+1 more reviewer

@jtmoon79 jtmoon79 jtmoon79 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

waiting-on-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pitdicker @djc @jtmoon79