-
Notifications
You must be signed in to change notification settings - Fork 517
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
Another open problem: clippy warns about integers starting with |
Can probably add clippy |
402fe1c
to
0cb1204
Compare
Fixed the clippy warning with help from @Alexendoo in rust-lang/rust-clippy#11472. |
0cb1204
to
b1769a6
Compare
|
b1769a6
to
c693f86
Compare
I have separated this into smaller commits. And by creating a temporary |
c693f86
to
972a08f
Compare
972a08f
to
6e3f9fc
Compare
Updated to remove the rust 1.57 workaround. These macro's can make tests nicer to read. Example commit: 8e4092f. |
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.
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. |
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.
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](https://private-user-images.githubusercontent.com/815261/300921072-c9865ade-2d55-4779-8931-25b31f4c6f60.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjM4ODU2ODgsIm5iZiI6MTcyMzg4NTM4OCwicGF0aCI6Ii84MTUyNjEvMzAwOTIxMDcyLWM5ODY1YWRlLTJkNTUtNDc3OS04OTMxLTI1YjMxZjRjNmY2MC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwODE3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDgxN1QwOTAzMDhaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0yZGViMTliNjYxYzQ0ODNkMzYyMjZhMmFmNjVmMDA5NmU3MDdiZmI4MDVlM2E2NWYyOTU4Y2E1MjE3MDNhNTEyJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.R1piJ7YRlHjR70eoC-yeeQ8L1isaZaW-tVWZ6QoPB-w)
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"); |
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 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"), |
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 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.
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.
Good comments, thanks @jtmoon79!
I'm not sure if we can use a format string for panics in const context, let me try.
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.
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
|
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.
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.
6e3f9fc
to
3126c6b
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.
Approved. However I like my idea of getting specific about precisely which argument made the date invalid. Up to you.
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(); |
Found a way to accept a time with fractions of a second using the So we can now also do @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. |
I am very happy to have something like this!
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 constDateTime<FixedOffset>
.Fixes #13.