-
Notifications
You must be signed in to change notification settings - Fork 715
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
4012 remove dependency to chrono #4020
Conversation
Leo-Besancon
commented
Jun 1, 2023
- document all added functions
- try in sandbox /simulation/labnet
- unit tests on the added/changed features
- make tests compile
- make tests pass
- add logs allowing easy debugging in case the changes caused problems
- if the API has changed, update the API specification
massa-bootstrap/src/client.rs
Outdated
.unwrap(); | ||
warn!("Please update your Massa node before: {}", dt.to_rfc2822()); | ||
|
||
let activation_datetime = OffsetDateTime::from_unix_timestamp( |
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 vote for encapsulating all time related stuff into MassaTime.
So basically we can have "MassaTime::format_instant" and a "MassaTime::format_duration" methods
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.
Done in f0c9d7e
massa-versioning/src/versioning.rs
Outdated
start: MassaTime::from_millis(start.timestamp() as u64), | ||
timeout: MassaTime::from_millis(timeout.timestamp() as u64), | ||
start: MassaTime::from_millis( | ||
start_datetime |
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.
we should use MassaTime everywhere. Why are we using NaiveDateTime and OffsetDateTime at all ?
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.
Changed to MassaTime in f0c9d7e
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.
LGTM