Skip to content

/rust-cookbook

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

Adds "converting a local time to UTC time and vice versa" recipe #364

Closed
wants to merge 0 commits into from

Conversation

Reviewers

@budziq budziq

@ludwigpacifici ludwigpacifici

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Lapz @budziq @ludwigpacifici
@Lapz
Copy link
Contributor

Lapz commented Nov 9, 2017 •
edited

fixes #360

@Lapz Lapz changed the title Fixes #360 Adds "converting a local time to UTC time and vice versa" recipe Nov 9, 2017

@budziq

budziq requested changes Nov 11, 2017
View changes

Copy link
Collaborator

budziq left a comment

Thanks @Lapz! The PR is almost perfect.
Just few suggestions below.
src/basics.md Outdated
[`HashMap`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html
[`hmac::Signature`]: https://docs.rs/ring/*/ring/hmac/struct.Signature.html
[`IndependentSample::ind_sample`]: https://doc.rust-lang.org/rand/rand/distributions/trait.IndependentSample.html#tymethod.ind_sample
[`Lines`]: https://doc.rust-lang.org/std/io/struct.Lines.html
[`Local::now`]:
https://docs.rs/chrono/0.4.0/chrono/offset/struct.Local.html#method.now

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator
  • please use wildcard version number
src/basics.md Outdated
@@ -1415,10 +1442,14 @@ fn main() {
[`File::open`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.open
[`File::try_clone`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.try_clone
[`File`]: https://doc.rust-lang.org/std/fs/struct.File.html
[`FixedOffset`]:

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator
  • please keep the reference links in a single line
  • I'd suggest to qualify the name with at least one more hierarchy segment
src/basics.md Outdated
[`HashMap`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html
[`hmac::Signature`]: https://docs.rs/ring/*/ring/hmac/struct.Signature.html
[`IndependentSample::ind_sample`]: https://doc.rust-lang.org/rand/rand/distributions/trait.IndependentSample.html#tymethod.ind_sample
[`Lines`]: https://doc.rust-lang.org/std/io/struct.Lines.html
[`Local::now`]:

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator
  • please keep the reference links in a single line
  • I'd suggest to qualify the name with at least one more hierarchy segment chrono::offset::Local::now or offset::Local::now
src/basics.md Outdated
@@ -1265,6 +1266,32 @@ fn main() {
println!("Time elapsed in expensive_function() is: {:?}", duration);
}
```
[ex-convert-datetime]: #ex-convert-datetime

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator

Lets indicate that timezones are used in the example identifier
src/basics.md Outdated
@@ -1265,6 +1266,32 @@ fn main() {
println!("Time elapsed in expensive_function() is: {:?}", duration);
}
```
[ex-convert-datetime]: #ex-convert-datetime
<a name="ex-convert-datetime"></a>
## Converting a local time to UTC time and vice versa

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator

Adding "timezone" to the title would go a long way in making it more discoverable
src/basics.md Outdated
## Converting a local time to UTC time and vice versa
[![chrono-badge]][chrono] [![cat-date-and-time-badge]][cat-date-and-time]

Gets the local time and displays it using [`Local::now`] and then converts it to the UTC format using the [`DateTime`] struct. A timezone is then constructed using the [`FixedOffset`] struct and the UTC time is the converted to UTC+8 and UTC-8.

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator
  • s/time is the converted/time is then converted/
  • "using the [DateTime] struct" could use a linked mention about DateTime::from_utc
src/basics.md Outdated
let local_time: DateTime<Local> = Local::now();
let utc_time: DateTime<Utc> = DateTime::<Utc>::from_utc(local_time.naive_utc(), Utc);
let china_timezone: FixedOffset = FixedOffset::east(28_800);
let la_timezone: FixedOffset = FixedOffset::west(28_800);

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 11, 2017

Collaborator

I'd suggest to add a little more variety to the example by changint the offset in either "east" or "west" wariant

@Lapz Lapz force-pushed the Lapz:master branch from c9e4a7d to ae7732a Nov 12, 2017

@Lapz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

Lapz commented Nov 12, 2017

@budziq I've made the necessary changes requested.
@budziq

budziq requested changes Nov 12, 2017
View changes

Copy link
Collaborator

budziq left a comment

Some final suggestions and we are good to merge.
src/basics.md Outdated
@@ -1265,6 +1266,32 @@ fn main() {
println!("Time elapsed in expensive_function() is: {:?}", duration);
}
```
[ex-convert-datetime-timezone]: #ex-convert-datetime-timezone
<a name="ex-convert-datetime-timezone"></a>
## Converting a local time to UTC time and vice versa

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

lets change the title to include "timezone" for better discoverability
src/basics.md Outdated
## Converting a local time to UTC time and vice versa
[![chrono-badge]][chrono] [![cat-date-and-time-badge]][cat-date-and-time]

Gets the local time and displays it using [`offset::Local::now`] and then converts it to the UTC format using the [`DateTime::from_utc`] struct method. A time is then converted using the [`offset::FixedOffset`] struct and the UTC time is the converted to UTC+8 and UTC-.

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator
  • UTC is a "standard" not a "format"
  • s/time is the converted/time is then converted/
  • "UTC-" is missing the value
src/basics.md Outdated
let china_timezone: FixedOffset = FixedOffset::east(28_800);
let rio_timezone: FixedOffset = FixedOffset::west(7_200);
println!("Local time now is {}", local_time);
println!("Utc time now is {}", utc_time);

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

s/Utc/UTC/
src/basics.md Outdated
@@ -1415,10 +1443,12 @@ fn main() {
[`File::open`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.open
[`File::try_clone`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.try_clone
[`File`]: https://doc.rust-lang.org/std/fs/struct.File.html
[`offset::FixedOffset`: https://docs.rs/chrono/*/chrono/offset/struct.FixedOffset.html

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

Please keep the reference links sorted
src/basics.md Outdated
[`HashMap`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html
[`hmac::Signature`]: https://docs.rs/ring/*/ring/hmac/struct.Signature.html
[`IndependentSample::ind_sample`]: https://doc.rust-lang.org/rand/rand/distributions/trait.IndependentSample.html#tymethod.ind_sample
[`Lines`]: https://doc.rust-lang.org/std/io/struct.Lines.html
[`offset::Local::now`]:https://docs.rs/chrono/*/chrono/offset/struct.Local.html#method.now

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

please keep the links sorted
src/intro.md Outdated
@@ -45,6 +45,7 @@ community. It needs and welcomes help. For details see
| [Check number of logical cpu cores][ex-check-cpu-cores] | [![num_cpus-badge]][num_cpus] | [![cat-hardware-support-badge]][cat-hardware-support] |
| [Obtain backtrace of complex error scenarios][ex-error-chain-backtrace] | [![error-chain-badge]][error-chain] | [![cat-rust-patterns-badge]][cat-rust-patterns] |
| [Measure elapsed time][ex-measure-elapsed-time] | [![std-badge]][std] | [![cat-time-badge]][cat-time] |
| [Converting a local time to UTC time and vice versa][ex-convert-datetime-timezone] | [![chrono-badge]][chrono] | [![cat-date-and-time-badge]][cat-date-and-time] |

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

Please use imperative form instead of continuous one in the example title.
s/Converting/Convert/
src/basics.md Outdated
@@ -27,6 +27,7 @@
| [Check number of logical cpu cores][ex-check-cpu-cores] | [![num_cpus-badge]][num_cpus] | [![cat-hardware-support-badge]][cat-hardware-support] |
| [Obtain backtrace of complex error scenarios][ex-error-chain-backtrace] | [![error-chain-badge]][error-chain] | [![cat-rust-patterns-badge]][cat-rust-patterns] |
| [Measure elapsed time][ex-measure-elapsed-time] | [![std-badge]][std] | [![cat-time-badge]][cat-time] |
| [Converting a local time to UTC time and vice versa][ex-convert-datetime-timezone] | [![chrono-badge]][chrono] | [![cat-date-and-time-badge]][cat-date-and-time] |

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

Please use imperative form instead of continuous one in the example title.
s/Converting/Convert/
src/basics.md Outdated
let utc_time: DateTime<Utc> = DateTime::<Utc>::from_utc(local_time.naive_utc(), Utc);
let china_timezone: FixedOffset = FixedOffset::east(28_800);
let rio_timezone: FixedOffset = FixedOffset::west(7_200);
println!("Local time now is {}", local_time);

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

It would be cool to show what is the local TZ offset (as well as printout the other TZ offsets)
But I'll leave it up to you.
@ludwigpacifici

ludwigpacifici reviewed Nov 13, 2017
View changes

src/basics.md Outdated
fn main() {
let local_time: DateTime<Local> = Local::now();
let utc_time: DateTime<Utc> = DateTime::<Utc>::from_utc(local_time.naive_utc(), Utc);
let china_timezone: FixedOffset = FixedOffset::east(28_800);

This comment has been minimized.

Sign in to view
Copy link
@ludwigpacifici

ludwigpacifici Nov 13, 2017 •
edited

Contributor

@budziq, To ease the understanding, is it worth to change 28_800 to 8 * 3600?

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 13, 2017

Collaborator

Looking at the api examples for east I see that the author thinks such information is nontrivial.
So I'm inclined to follow their convention 👍
@ludwigpacifici

ludwigpacifici reviewed Nov 13, 2017
View changes

src/basics.md Outdated
let local_time: DateTime<Local> = Local::now();
let utc_time: DateTime<Utc> = DateTime::<Utc>::from_utc(local_time.naive_utc(), Utc);
let china_timezone: FixedOffset = FixedOffset::east(28_800);
let rio_timezone: FixedOffset = FixedOffset::west(7_200);

This comment has been minimized.

Sign in to view
Copy link
@ludwigpacifici

ludwigpacifici Nov 13, 2017 •
edited

Contributor

@budziq, To ease the understanding, is it worth to change 7_200 to 2 * 3600?
@ludwigpacifici

ludwigpacifici reviewed Nov 13, 2017
View changes

src/basics.md Outdated
fn main() {
let local_time: DateTime<Local> = Local::now();
let utc_time: DateTime<Utc> = DateTime::<Utc>::from_utc(local_time.naive_utc(), Utc);

This comment has been minimized.

Sign in to view
Copy link
@ludwigpacifici

ludwigpacifici Nov 13, 2017

Contributor

@budziq, is it worth it to specify all the types for the variable? For me, it does not bring useful information and makes the example slightly shorter.

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 13, 2017 •
edited

Collaborator

If the information could be inferred the any type parameters or annotations should be removed for brevity. Good catch!
@budziq

This comment has been minimized.

Sign in to view
Copy link
Collaborator

budziq commented Nov 14, 2017

@Lapz there are still some unaddressed review comments left. Would you be able to look into those and squash+rebase the commit to latest master (without the merge commit)?

@Lapz Lapz closed this Nov 14, 2017

@Lapz Lapz force-pushed the Lapz:master branch from 508d2c0 to 2ab0ba6 Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.