-
Notifications
You must be signed in to change notification settings - Fork 240
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 RTC examples & expand RealTimeClock & ClockManager capabilities. #676
Conversation
I like those examples. Properly configuring the dormant state is non-trivial, so having a working example is really useful. Perhaps you could add a comment with the results of the measurements you mentioned in #659 (comment) ? |
#[cfg_attr(feature = "chrono", path = "datetime_chrono.rs")] | ||
#[cfg_attr(not(feature = "chrono"), path = "datetime_no_deps.rs")] |
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.
This is a breaking change and should be kept separate from this PR.
I don't know the full implications of this change. The conditional compilation was introduced by #213. @VictorKoenders and @9names did work on that PR. Can you comment on whether this conditional chrono support has proven useful in the mean time?
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.
For the motivation of that change in this PR, this feature currently changes the shape of the API.
This makes the rtc examples require conditional compilation to accommodate with the different DateTime creation style.
Following the philosophy of features being additive, it seems more natural that enabling the chrono
feature adds chrono support rather than replace the local types.
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.
That's a very good point for making the no-deps implementation available unconditionally. But in it's current form, this PR completely removes chrono support. (It leaves the datetime_chrono.rs
file in place, but there's no longer any code referencing it.)
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.
This block should be enough to allow easy conversion between chrono's NaiveDateTime
and our own DateTime
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.
@jonathanpallant recommended using chrono
in the previous PR #207, which is why it was in #213
4ea61e9
to
78e79ae
Compare
@jonathanpallant do you think the approach proposed here is acceptable? |
No description provided.