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

issue #361 fixed #365

Merged
merged 4 commits into from Nov 16, 2017

Conversation

Reviewers

@budziq budziq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@pduzinki @budziq
@pduzinki
Copy link
Contributor

pduzinki commented Nov 12, 2017 •
edited

fixes #361
@pduzinki
issue #361 fixed
Loading status checks…
055d251
@budziq

budziq requested changes Nov 12, 2017
View changes

Copy link
Collaborator

budziq left a comment

Thanks @pduzinki, the PR is almost perfect. Some suggestions below:
src/basics.md Outdated
@@ -1266,6 +1267,37 @@ fn main() {
}
```

[ex-convert-datetime-timestamp]: #ex-convert-datetime-timestamp
<a name="ex-convert-datetime-timestamp"></a>
## Converting date to unix timestamp and vice versa

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/
  • UNIX is all caps
src/basics.md Outdated
@@ -1423,8 +1455,12 @@ fn main() {
[`Mutex`]: https://doc.rust-lang.org/std/sync/struct.Mutex.html
[`MutexGuard`]: https://doc.rust-lang.org/std/sync/struct.MutexGuard.html
[`NaiveDate`]: https://docs.rs/chrono/*/chrono/naive/struct.NaiveDate.html
[`NaiveDate::from_ymd`]: https://docs.rs/chrono/0.4.0/chrono/naive/struct.NaiveDate.html#method.from_ymd

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

please use wildcard version number
src/basics.md Outdated
[`NaiveDateTime`]: https://docs.rs/chrono/*/chrono/naive/struct.NaiveDateTime.html
[`NaiveDateTime::from_timestamp`]: https://docs.rs/chrono/0.4.0/chrono/naive/struct.NaiveDateTime.html#method.from_timestamp

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator
  • please use wildcard version number
  • please keep link defs within a single line
src/basics.md Outdated
[`NaiveTime`]: https://docs.rs/chrono/*/chrono/naive/struct.NaiveTime.html
[`NaiveTime::from_hms`]: https://docs.rs/chrono/0.4.0/chrono/naive/struct.NaiveTime.html#method.from_hms

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

please use wildcard version number
src/basics.md Outdated
fn main() {
let date_time: NaiveDateTime = NaiveDate::from_ymd(2017, 11, 12).and_hms(17, 33, 44);
let seconds_since_date_time: i64 = date_time.timestamp();

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator
  • type ascription is not required here.
  • Also the variable name is incorrect. its seconds between UNIX Epoch and the date_time. Lets just drop the intermediate variable and call timestamp within the println body
src/basics.md Outdated
"Number of seconds between 1970-01-01 00:00:00 and {} is {}.",
date_time, seconds_since_date_time);
let billion_seconds: i64 = 1_000_000_000;

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

lets drop the intermediate variable here
src/basics.md Outdated
let billion_seconds: i64 = 1_000_000_000;
let date_time_after_a_billion_seconds = NaiveDateTime::from_timestamp(billion_seconds, 0);
println!(
"Date after a billion seconds from 1970-01-01 00:00:00 was {}.",

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

s/from/since/
src/basics.md Outdated
}
```


[ex-format-datetime]: #ex-format-datetime

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

one newline is enough
src/basics.md Outdated
[![chrono-badge]][chrono] [![cat-date-and-time-badge]][cat-date-and-time]

Converts a date given by [`NaiveDate::from_ymd`] and [`NaiveTime::from_hms`]
to the number of seconds since January 1, 1970 0:00:00 UTC using [`NaiveDateTime::timestamp`].

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 12, 2017

Collaborator

I'd suggest mentioning hyperlinked keywords such as UNIX and Timestamp
@pduzinki
review fixes
Loading status checks…
f008776
@budziq

budziq reviewed Nov 15, 2017
View changes

src/basics.md Outdated
Converts a date given by [`NaiveDate::from_ymd`] and [`NaiveTime::from_hms`]
to [UNIX timestamp] using [`NaiveDateTime::timestamp`].
Then it calculates what was the date after one billion seconds
since [UNIX timestamp], using [`NaiveDateTime::from_timestamp`].

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Nov 15, 2017

Collaborator

since [UNIX timestamp],

UNIX Timestamp itself is number of seconds since UNIX Epoch to this statement should be:
"Then it calculates what was the date after one billion seconds since [UNIX Epoch]" or "since 1970-01-01 00:00:00" as was previously.

This comment has been minimized.

Sign in to view
Copy link
@pduzinki

pduzinki Nov 15, 2017

Author Contributor

That's right, thanks. I'll change it back then.
@budziq

budziq requested changes Nov 15, 2017
View changes

Copy link
Collaborator

budziq left a comment

@pduzinki final review comment and we are ready to merge 👍

pduzinki added some commits Nov 15, 2017

@pduzinki
review fixes
Loading status checks…
70249cd
@pduzinki
Merge branch 'master' of https://github.com/rust-lang-nursery/rust-co… 
…okbook

# Conflicts:
#	src/basics.md
#	src/intro.md
Loading status checks…
0c7466d

@budziq budziq merged commit e668fff into rust-lang-nursery:master Nov 16, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Sign in to view
Copy link
Collaborator

budziq commented Nov 16, 2017

Thanks @pduzinki !
@budziq

This comment has been minimized.

Sign in to view
Copy link
Collaborator

budziq commented Nov 16, 2017

@pduzinki please comment on #209 to express your consent (or lack of it) for the repo license change
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.