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

Add easy initialization macros #1270

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Sep 8, 2023

I am very happy to have something like this!

// NaiveDate
date!(2023-09-08); // calendar date
date!(2023-251); // ordinal date

// NaiveTime
time!(7:03); // seconds are optional
time!(7:03:00);
time!(23:59:60); // leap second

// NaiveDateTime
datetime!(2023-09-08 7:03);
datetime!(2023-09-08 7:03:25);

// DateTime<FixedOffset>
datetime!(2023-09-08 7:03:25+02:00);
datetime!(2023-09-08 7:03:25-02:00);

My macro skills are not great though. I do not know how to avoid the code duplication. And I can't make the time! macro support fractional seconds yet. Something TT muncher something.

Depends on #1069 for creating a const DateTime<FixedOffset>.

Fixes #13.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: Patch coverage is 71.92982% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 91.67%. Comparing base (6c4e735) to head (2d52c4b).

Files Patch % Lines
src/macros.rs 71.92% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
- Coverage   91.79%   91.67%   -0.13%     
==========================================
  Files          37       38       +1     
  Lines       18185    18299     +114     
==========================================
+ Hits        16693    16775      +82     
- Misses       1492     1524      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker
Copy link
Collaborator Author

Another open problem: clippy warns about integers starting with 0, which is common in date and time values.

@djc
Copy link
Contributor

djc commented Sep 8, 2023

Another open problem: clippy warns about integers starting with 0, which is common in date and time values.

Can probably add clippy #[allow()] attributes in the macro output?

@pitdicker
Copy link
Collaborator Author

Fixed the clippy warning with help from @Alexendoo in rust-lang/rust-clippy#11472.

@pitdicker
Copy link
Collaborator Author

  • The macro to create a DateTime<FixedOffset> hits the same problem as in Make methods on NaiveDateTime const where possible #1286 (comment): Rust 1.57 doesn't support making DateTime<Tz>::from_naive_utc_and_offset const.
    Added a workaround.
  • We can't easily see whether a macro is const, so I added a test for it.
  • Added an offset! macro. offset!(+05:43) is a lot easier to read then FixedOffset::east_opt(20_580).unwrap().

@pitdicker
Copy link
Collaborator Author

I have separated this into smaller commits.

And by creating a temporary const inside the macros we can guarantee the values are always checked at compile time.

@pitdicker
Copy link
Collaborator Author

Updated to remove the rust 1.57 workaround.

These macro's can make tests nicer to read. Example commit: 8e4092f.
They give us parity with one of the selling points of the time crate.

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.

These are very handy!

I have some suggestions to improve user-friendliness.

src/macros.rs Outdated
@@ -0,0 +1,316 @@
//! Macro's for easy initialization of date and time values.

/// Create a `NaiveDate` with a statically known value.
Copy link
Contributor

@jtmoon79 jtmoon79 Jan 30, 2024

Choose a reason for hiding this comment

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

Link the NaiveDate.

Since the macro does not have a return type the docs will not automatically link to NaiveDate.

So change to a link

 /// Create a [`NaiveDate`] with ...

at the bottom, add the link target

/// [`NaiveDate`]: crate::naive::NaiveDate

btw, if you use VS Code, it will highlight the link to turquoise blue when it is correctly set.

Screenshot 2024-01-30 102402

This nitpick goes for the rest of the PR.

(-$h:literal:$m:literal) => {{
#[allow(clippy::zero_prefixed_literal)]
{
assert!($h < 24 || $m < 60, "invalid offset");
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend splitting these out to be more specific what value is wrong. Also, reprint the wrong value. This helps the user quickly and easily identify the culprit.

 assert!($h < 24, "invalid hour offset {}", $h);
 assert!($m < 60, "invalid minute offset {}", $m);

Apply this comment to rest of the PR.

{
const DATE: $crate::NaiveDate = match $crate::NaiveDate::from_ymd_opt($y, $m, $d) {
Some(d) => d,
None => panic!("invalid calendar date"),
Copy link
Contributor

@jtmoon79 jtmoon79 Jan 30, 2024

Choose a reason for hiding this comment

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

I prefer to repeat back to the user the invalid values. More specific clues in error messages are always nice.

 panic!("invalid calendar date from values {:?}, {:?}, {:?}", $y, $m, $d)

Apply this comment to rest of the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good comments, thanks @jtmoon79!
I'm not sure if we can use a format string for panics in const context, let me try.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because these macro's are evaluated at compile time the message is already quite nice in my opinion:

error[E0080]: evaluation of constant value failed
  --> src\macros.rs:52:20
   |
52 |         assert_eq!(date!(2023-04-31), NaiveDate::from_ymd_opt(2023, 4, 30).unwrap());
   |                    ^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'invalid calendar date', src\macros.rs:52:20
   |

Copy link
Contributor

Choose a reason for hiding this comment

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

Because these macro's are evaluated at compile time the message is already quite nice in my opinion:

error[E0080]: evaluation of constant value failed
  --> src\macros.rs:52:20
   |
52 |         assert_eq!(date!(2023-04-31), NaiveDate::from_ymd_opt(2023, 4, 30).unwrap());
   |                    ^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'invalid calendar date', src\macros.rs:52:20
   |

I still think a nice touch here is to be specific about precisely which argument caused the panic. Up to you.

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.

Approved. However I like my idea of getting specific about precisely which argument made the date invalid. Up to you.

@sorairolake
Copy link

I think it would be useful to have an initialization macro for DateTime<Utc>.

// DateTime<Utc>
datetime!(2023-09-08 7:03:25Z);

// DateTime<FixedOffset>
datetime!(2023-09-08 7:03:25+00:00);

@pitdicker
Copy link
Collaborator Author

I think it would be useful to have an initialization macro for DateTime<Utc>.

Agreed. But as I understand such a syntax, using a letter after a number, is not supported by simple macro's. And my goal was to keep it simple, without resorting to proc macro's.

Note that you can do

// DateTime<Utc>
datetime!(2023-09-08 7:03:25).and_utc();

@pitdicker
Copy link
Collaborator Author

Found a way to accept a time with fractions of a second using the stringify macro!

So we can now also do time!(7:03:15.01), and the same for datetime!().

@djc What would be needed to push this PR forwards? I think it will be a really good addition to improve the ergonomics of chrono.

b.t.w. I opened rust-lang/rust#123156 for the CI errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialization macros
4 participants