Add example for checked arithmetic of chrono #352

hegza · Oct 31, 2017

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici · Nov 2, 2017

src/basics.md

+[`DateTime::checked_sub_signed`]. The methods return None if the date and time
+cannot be calculated.
+
+Escape sequences that are available for the


Display formatted date and time is already described in the cookbook. if you want to have one line explaining this, I would forward the reader to the relevant recipe.

If I understood correctly what you meant, I feel like this would indeed reduce redundancy, but it would also disorient the reader. Here, it cannot be assumed that the reader is familiar with all the recipes. That's why I would probably prefer to just repeat something as simple as this, rather than link to another recipe.

If I understood correctly what you meant

Sorry for not being clear. You guessed it right :-)

My thoughts behind forwarding the reader: it makes the cookbook more discoverable (help the reader to be familiar with several recipes) and each recipe focuses solely one item (less redundancy).

I let you discuss it with @budziq.

Currently we are analyzing how to reorganize the cookbook (including some form of recipe interconnection). For now I would not sweat that much about it as all the recipes will get an editorial pass in near future once we decide on a final form.

For now I'd just remove the formatting from the example and description all of the dates hare can be printed with just println!("{}", yesterday);

ludwigpacifici · Nov 2, 2017

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici · Nov 2, 2017

src/basics.md

+    if let Some(two_weeks_from_now) = now.checked_add_signed(Duration::weeks(2)) {
+        println!("Two weeks from now is {}", two_weeks_from_now);
+    } else {
+        println!("Two weeks from now overflows!");


Usually errors are handled via error_chain crate. Not sure here it would make the code shorter. However, one way to do it is to convert the Option returned by checked_add_signed and convert it into an error via ok_or and use ? to bubble up the error. Also the code has to be moved into a function fn run() -> Result<()>. Check other recipes for this kind of example.

Another way to improve errors is use eprintln! instead of println!.

I did wonder a bit about chrono's decision to use Option here instead of Result. I think Result would be more correct here. But as it is, I felt that it's not appropriate to use error_chain with Option and went for the pattern matching approach.

I would love to know if my reasoning has got something to improve here. I will definitely do something about it, at least add the eprintln!, but do tell if you still have something to add to this.

I did wonder a bit about chrono's decision to use Option here instead of Result.

I agree with you. And I think it's worth to discuss it with the chrono team. there is a similar talk.

do tell if you still have something to add to this.

I though of a third solution, you keep the if let Some and remove the else statement. However, it's an ugly because it hides errors.

In order to be homogeneous with the rest of the cookbook and be slightly more concise, I'll go with the error_chain solution.

@budziq any thoughts?

I did wonder a bit about chrono's decision to use Option

It follows from the stdlibs decision about checked integer arithmetic. I agree that both of these decisions are debatable.

If the Option None variant here is an Err is somewhat context dependent (personally I'd just go with match here instead of if let Some() or error_chain in the default case) but if we build some additional narrative (ie. put the calculations into a well named function indicating that it could fail) I think that we could justify error_chain usage. What are you thoughts?

I'd also suggest adding a series of calculations with and_then combinator to spice up the example.

ludwigpacifici · Nov 2, 2017

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici · Nov 2, 2017

src/basics.md

+    if let Some(yesterday) = now.checked_sub_signed(Duration::days(1)) {
+        println!("Yesterday was {}", yesterday.format("%a %b %e %Y"));
+    } else {
+        println!("Yesterday overflows!");


Same as my previous comment.

ludwigpacifici · Nov 2, 2017

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici · Nov 2, 2017

src/basics.md

+            heat_death
+        );
+    } else {
+        println!("We can't use chrono to tell the date of the heat death of the universe.");


Same as my previous comment.

ludwigpacifici · Nov 2, 2017

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici · Nov 2, 2017

src/basics.md

@@ -1297,6 +1342,9 @@ fn main() {
 [`DateTime::to_rfc2822`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.to_rfc2822
 [`DateTime::to_rfc3339`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.to_rfc3339
 [`DateTime::format`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.format
+[`DateTime::checked_add_signed`]: https://docs.rs/chrono/*/chrono/struct.Date.html#method.checked_add_signed


Please, sort links.

non rendered items are in sorted order (links, reference, identifiers, Cargo.toml.

from pull request template

The links in that whole section are generally unordered. That's why I followed chronological ordering grouped by topic / crate. Should I perhaps alpha-sort the whole list of API reference links, or just the ones belonging to the chrono crate?

Can you alpha-sort the whole API Reference list in a separate commit in this PR?

Yes, can do. I'll get on it as soon as I'm able.

I've already prepared a PR sorting the links as a part of the usual upkeep #355 (~~will merge it once CI pass is finished~~ merged) Sadly I't it will conflict with your commit but the conflict should be minimal.

No problem.

ludwigpacifici · Nov 2, 2017

Thanks @hegza for your PR! I suggested some modifications. Tell me what you think. Wait for @budziq for his official review.

One last comment about your commit message, can you reword it to follow the below:

PR contains correct "fixes #ISSUE_ID" clause to autoclose the issue once PR is merged

from pull request template.

hegza · Nov 2, 2017

The PR does not fix any specific issue, although it is related to, and partially fixes issue #324. In this case, I would prefer not to put an auto-closing tag. If I happen to be the person to commit the last fix to that issue #324, I promise to format the commit message accordingly ^^

ludwigpacifici · Nov 2, 2017

My mistake, I though you were solving a specific issue. In that case, it make sense to not autoclose the issue :-)

@budziq is not inconvenient in case someone can autoclose a todo list by solving one item? Is it worth it to always add a specific issue for solving one item of the list?

budziq · Nov 2, 2017

budziq requested changes Nov 2, 2017

View changes

Thanks @hegza and @ludwigpacifici !

Some comments below

budziq · Nov 2, 2017

src/basics.md

+[`DateTime::checked_sub_signed`]. The methods return None if the date and time
+cannot be calculated.
+
+Escape sequences that are available for the


Currently we are analyzing how to reorganize the cookbook (including some form of recipe interconnection). For now I would not sweat that much about it as all the recipes will get an editorial pass in near future once we decide on a final form.

For now I'd just remove the formatting from the example and description all of the dates hare can be printed with just println!("{}", yesterday);

budziq · Nov 2, 2017

src/basics.md

@@ -1297,6 +1342,9 @@ fn main() {
 [`DateTime::to_rfc2822`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.to_rfc2822
 [`DateTime::to_rfc3339`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.to_rfc3339
 [`DateTime::format`]: https://docs.rs/chrono/*/chrono/struct.DateTime.html#method.format
+[`DateTime::checked_add_signed`]: https://docs.rs/chrono/*/chrono/struct.Date.html#method.checked_add_signed


I've already prepared a PR sorting the links as a part of the usual upkeep #355 (~~will merge it once CI pass is finished~~ merged) Sadly I't it will conflict with your commit but the conflict should be minimal.

budziq · Nov 2, 2017

src/basics.md

+
+    if let Some(heat_death) = now.checked_add_signed(Duration::max_value()) {
+        println!(
+            "Heat death of the universe: {}",


Heat death of the universe is estimated at ~10e103 years while Duration::max_value() is 2^63 (i64::MAX) in milliseconds making it roughly 2.9243e8 years. So our statement would be a little inaccurate (94 orders of magnitude give or take ;)
Which is sad cause the idea was awesome :)

I would suggest to change it to something like ("maximal representable time" ) unless you come up with something equally awesome as heat death of the universe.

Yeah, I tried to do it with the correct numbers at first, but realized that I'd have to delegate the heavy-lifting to a new crate, and that was a place I wasn't prepared to go to. :D I'll figure it out.

budziq · Nov 2, 2017

src/basics.md

+    }
+
+    if let Some(yesterday) = now.checked_sub_signed(Duration::days(1)) {
+        println!("Yesterday was {}", yesterday.format("%a %b %e %Y"));


lets simplify the example and description by removing the format

budziq · Nov 2, 2017

src/basics.md

+    if let Some(two_weeks_from_now) = now.checked_add_signed(Duration::weeks(2)) {
+        println!("Two weeks from now is {}", two_weeks_from_now);
+    } else {
+        println!("Two weeks from now overflows!");


I did wonder a bit about chrono's decision to use Option

It follows from the stdlibs decision about checked integer arithmetic. I agree that both of these decisions are debatable.

If the Option None variant here is an Err is somewhat context dependent (personally I'd just go with match here instead of if let Some() or error_chain in the default case) but if we build some additional narrative (ie. put the calculations into a well named function indicating that it could fail) I think that we could justify error_chain usage. What are you thoughts?

I'd also suggest adding a series of calculations with and_then combinator to spice up the example.

hegza · Nov 2, 2017

I made another iteration. Feel free to check it out. I'll be on stand by until the merges are required.

budziq · Nov 3, 2017

@ludwigpacifici

@budziq is not inconvenient in case someone can autoclose a todo list by solving one item? Is it worth it to always add a specific issue for solving one item of the list?

Eh my bad. The "fixes" clause is only meant for PR's addressing a specific "example idea" issues but its not stated anywhere (mostly due to PR template being overly verbose and restrictive already). If anyone has a suggestion to make it clearer in a form that is not even more TL;NR I'm all ears :) In general I see no point in autoclosing the tracking issues as these are meant to gather example ideas from readers and contributors.

Also my bad again for not making the "example idea" issues for chrono untill now haven't had time much time near PC lately (currently writing on mobile).

budziq · Nov 3, 2017

budziq requested changes Nov 3, 2017

View changes

@hegza few minor comments below and we are ready to merge!
I've already fixed conflicts and merged your previous PR 👍

budziq · Nov 3, 2017

src/basics.md

+    let time_until_full_galactic_orbit = now.checked_add_signed(Duration::max_value());
+    match time_until_full_galactic_orbit {
+        Some(x) => println!("{}", x),
+        None => println!("We can't use chrono to tell the date of the Solar System completing one full orbit around the galactic center."),


Super cool! 👍
Two points of note:

this would not be actually a "date" but "time" (the date range is actually much shorter)

we actually can represent One Galactic year (it is somewhere in the range of 2.25e8 - 2.50e8 years) and we can represent upto ~2.92e8 years. Lets rewrite it as

We can't use chrono to tell the time for the Solar System to comple more than one full orbit around the galactic center.

Sorry for being so anal about this :)

I would expect no less scientific accuracy here. My mistake :D

budziq · Nov 3, 2017

src/basics.md

+extern crate chrono;
+use chrono::{DateTime, Duration, Utc};
+
+fn yesterday(date_time: DateTime<Utc>) -> Option<DateTime<Utc>> {


how about renaming it to day_earlier making it a little more accurate as we are not necessarily anchored in now?

A very good idea. I wanted something like that but wasn't able to properly express myself.

budziq · Nov 3, 2017

src/basics.md

+    println!("{}", now);
+
+    let two_weeks_from_now = now.checked_add_signed(Duration::weeks(2));
+    match two_weeks_from_now {


how about pulling this calculation directly into the almost_three_weeks_from_now variable or at least dropping the whole match+printout here as the example gets a little noisy with it.

budziq · Nov 3, 2017

src/basics.md

+    }
+
+    let almost_three_weeks_from_now =
+            two_weeks_from_now.unwrap()


Lets not use unwrap in our examples this is exactly why and_then was introduced 👍

let almost_three_weeks_from_now = now.checked_add_signed(Duration::weeks(2)) .and_then(|in_2weeks| in_2weeks.checked_add_signed(Duration::weeks(1))) .and_then(day_earlier);

budziq · Nov 3, 2017

src/basics.md

+            .checked_add_signed(Duration::weeks(1)).and_then(yesterday);
+    match almost_three_weeks_from_now {
+        Some(x) => println!("{}", x),
+        None => println!("Almost three weeks from now overflows!"),


if we are simulating onsite error handling lets use eprintln!

budziq · Nov 3, 2017

src/basics.md

+    }
+
+    let time_until_full_galactic_orbit = now.checked_add_signed(Duration::max_value());
+    match time_until_full_galactic_orbit {


how about compacting these two lines into one?

match now.checked_add_signed(Duration::max_value()) {

budziq · Nov 3, 2017

src/basics.md

+
+    let almost_three_weeks_from_now =
+            two_weeks_from_now.unwrap()
+            .checked_add_signed(Duration::weeks(1)).and_then(yesterday);


I would introduce a newline between the variable creation and match to improve the readability.

ludwigpacifici · Nov 3, 2017

FYI, @hegza you have your dedicated issue: #359. If you don't mind to reword your commit message with your next PR update :-)

hegza · Nov 4, 2017

Fixed, reworded. Looking fine now, to me at least. Drop me a comment if you disagree!

budziq · Nov 4, 2017

budziq requested changes Nov 4, 2017

View changes

Last two cosmetic changes and we are ready to merge 👍

budziq · Nov 4, 2017

src/basics.md

+
+    match now.checked_add_signed(Duration::max_value()) {
+        Some(x) => println!("{}", x),
+        None => println!("We can't use chrono to tell the time for the Solar System to complete more than one full orbit around the galactic center."),


Lets use eprintln! here consistently with the previous match and we are ready to merge. I'd do it myself by I'm mobile only this weekend.

Oh, weird that I missed that while updating the other eprintln!

budziq · Nov 4, 2017

src/basics.md

@@ -1343,6 +1344,47 @@ fn run() -> Result<()> {
 # quick_main!(run);
 ```

+[ex-datetime-arithmetic]: #ex-datetime-arithmetic
+<a name="ex-datetime-arithmetic"></a>
+## Add or subtract dates and times using checked arithmetic


I'd suggest renaming the example to "Perform checked date and time calculations". It will be slightly more discoverable.

hegza · Nov 4, 2017

Ready once more.

budziq · Nov 4, 2017

Thanks for sticking with it. Nicely done @hegza 👍
And thanks for help with the review @ludwigpacifici :)

hegza force-pushed the hegza:feature/chrono-arithmetic branch from 7c9e5d4 to 5566721 Nov 2, 2017

hegza force-pushed the hegza:feature/chrono-arithmetic branch 5 times, most recently from b0cbb01 to 9783b94 Nov 4, 2017

Add example for checked arithmetic of chrono
Fixes #359 See also: #324

Loading status checks…

b153c29

hegza force-pushed the hegza:feature/chrono-arithmetic branch from 9783b94 to b153c29 Nov 4, 2017

budziq merged commit 2ab0ba6 into rust-lang-nursery:master Nov 4, 2017
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

hegza deleted the hegza:feature/chrono-arithmetic branch Nov 5, 2017

rust-lang-nursery/rust-cookbook

Join GitHub today

Add example for checked arithmetic of chrono #352

Conversation

hegza commented Oct 31, 2017 • edited by budziq

ludwigpacifici reviewed Nov 2, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ludwigpacifici reviewed Nov 2, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ludwigpacifici reviewed Nov 2, 2017 View changes

This comment has been minimized.

ludwigpacifici reviewed Nov 2, 2017 View changes

This comment has been minimized.

ludwigpacifici reviewed Nov 2, 2017 View changes

This comment has been minimized.

This comment has been minimized.

hegza Nov 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq Nov 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

ludwigpacifici commented Nov 2, 2017

This comment has been minimized.

hegza commented Nov 2, 2017

This comment has been minimized.

ludwigpacifici commented Nov 2, 2017

budziq requested changes Nov 2, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

budziq Nov 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hegza force-pushed the hegza:feature/chrono-arithmetic branch from 7c9e5d4 to 5566721 Nov 2, 2017

This comment has been minimized.

hegza commented Nov 2, 2017

This comment has been minimized.

budziq commented Nov 3, 2017

budziq requested changes Nov 3, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ludwigpacifici commented Nov 3, 2017 • edited

hegza force-pushed the hegza:feature/chrono-arithmetic branch 5 times, most recently from b0cbb01 to 9783b94 Nov 4, 2017

This comment has been minimized.

hegza commented Nov 4, 2017

budziq requested changes Nov 4, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hegza force-pushed the hegza:feature/chrono-arithmetic branch from 9783b94 to b153c29 Nov 4, 2017

This comment has been minimized.

hegza commented Nov 4, 2017

Hide details View details budziq merged commit 2ab0ba6 into rust-lang-nursery:master Nov 4, 2017 1 of 2 checks passed

1 of 2 checks passed

This comment has been minimized.

budziq commented Nov 4, 2017

hegza deleted the hegza:feature/chrono-arithmetic branch Nov 5, 2017

hegza commented Oct 31, 2017 •

edited by budziq

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici reviewed Nov 2, 2017

View changes

ludwigpacifici reviewed Nov 2, 2017

View changes

hegza Nov 2, 2017 •

edited

budziq Nov 2, 2017 •

edited

budziq requested changes Nov 2, 2017

View changes

budziq Nov 2, 2017 •

edited

hegza force-pushed the hegza:feature/chrono-arithmetic branch from `7c9e5d4` to `5566721` Nov 2, 2017

budziq requested changes Nov 3, 2017

View changes

ludwigpacifici commented Nov 3, 2017 •

edited

hegza force-pushed the hegza:feature/chrono-arithmetic branch 5 times, most recently from `b0cbb01` to `9783b94` Nov 4, 2017

budziq requested changes Nov 4, 2017

View changes

hegza force-pushed the hegza:feature/chrono-arithmetic branch from `9783b94` to `b153c29` Nov 4, 2017

budziq merged commit `2ab0ba6` into rust-lang-nursery:master Nov 4, 2017
1 of 2 checks passed