-
Notifications
You must be signed in to change notification settings - Fork 122
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
Timezone support #970
Timezone support #970
Conversation
I should add more tests. |
lib/time.rs
Outdated
} | ||
|
||
pub fn time_format(t: &Time, format: &String) -> String { | ||
t.val.format(format) | ||
t.val.format(format).to_string() |
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 will panic if format
is invalid. I think this should return Result<>
.
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.
Maybe it should return Result, but the underlying type has no such API.
This issue seems to be 6 years old: chronotope/chrono#47
Do you know of another API to use?
lib/time.rs
Outdated
pub fn date_format(d: &Date, format: &String) -> String { | ||
d.val.format(format) | ||
d.val.format(format).to_string() |
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.
Same here, need to handle errors gracefully.
lib/time.rs
Outdated
let dt = ::time::PrimitiveDateTime::new((*d).date.val, (*d).time.val); | ||
dt.format(format) | ||
let dt = ::chrono::NaiveDateTime::new((*d).date.val, (*d).time.val); | ||
dt.format(format).to_string() |
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.
unsafe
@@ -778,6 +778,7 @@ mkCargoToml rs_code crate crate_id = | |||
"once_cell = \"1.4.1\"" $$ | |||
"libc = \"0.2\"" $$ | |||
"time = { version = \"0.2\", features = [\"serde\"] }" $$ | |||
"chrono = { version = \"0.4\", features = [\"serde\"] }" $$ |
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 shouldn't be here. The correct way to import library dependencies is by creating a time.toml
file in the same directory as time.dl
. The time
crate is special in that they it is also used by some of the framework code, and so must be imported by all generated crates.
I have also realized that no library files had copyright.
The previous implementation of time was based on a Rust crate called time, which used non-standard format specifiers and did not support time zones. So the implementation has been rewritten based on the chrono crate which uses C strftime format. However, the names of the functions were not changed, to prevent client breakage. The old library gave better error messages on malformed times.