gigasecond: Update dependency to conform to usage guidelines #436

cbzehner · 2018-02-25T22:16:15Z

This updates the dependencies for the gigasecond exercise to conform to the usage guidelines for the chrono cargo package:

Avoid using use chrono::*; as Chrono exports several modules other than types. If you prefer the glob imports, use the following instead:

I realize using chrono::* might have been an intentional decision to avoid having to expose beginners to importing time::Duration as well, but I think it probably makes more sense to mirror a real environment closely when exposing people to these exercises.

I tried to make the minimum number of changes here and ensured that _test/check-exercises.sh was able to validate the gigasecond exercise before submitting.

coriolinus

Great idea, and thanks for this PR, @cbzehner!

There are a few things I'd like to see improved here before merging. Those are listed below.

coriolinus · 2018-02-25T22:53:58Z

exercises/gigasecond/Cargo.toml

@@ -4,4 +4,5 @@ version = "1.1.0"

 [dependencies]
 chrono = "0.4"
+time = "0.1"


The time crate has been depreciated. We shouldn't be adding new dependencies to it. Instead, let's use the chrono reexport of Duration; that way, when that library updates to use std::time::Duration, we upgrade with it for free.

coriolinus · 2018-02-25T22:57:07Z

exercises/gigasecond/src/lib.rs

@@ -1,7 +1,10 @@
-extern crate chrono;
-use chrono::*;


Agree that we should remove the glob import as bad style, but I think we can do better. What about:

use chrono::{DateTime, Duration};

This way we don't have to mess with a second dependency or globs.

This applies both to the lib stub and the example.

cbzehner · 2018-02-26T00:04:14Z

Ah! That's much cleaner. I didn't realize time was deprecated. I'll update this PR with your suggestions.

cbzehner · 2018-02-26T17:20:38Z

exercises/gigasecond/src/lib.rs

-    unimplemented!()
-}
+extern crate chrono;
+use chrono::{DateTime, Duration, Utc};


Duration is an unused import here, but when I ran cargo check from exercises/gigasecond it didn't output any warnings so I left in in since it's a useful hint. Lmk what you think of this.

I have a mild preference to exclude Duration based only on an instinct that it's good for students to have to look at the docs at least a little bit. By providing the appropriate dependency crate and the stub which we have, we've already reduced the required work from the student to near nothing.

I'm willing to be overridden on this if people feel strongly otherwise, though.

The original angle that I wanted this exercise to take was to be some sort of introduction to crates (specifically for Rust) in addition to time/duration (applies to this exercise no matter what the language). That is what I thought in #79 (comment). Note that at the time there was no stub at all. I may have forgotten about that when reviewing #306 which added the stub. Encouraging a look at the docs seems good. As demonstrated by my having implicitly changed my mind about this a few times over the years, I don't feel strongly about it.

I say remove it but add a note to the hints file with a link to the documentation, so students who get lost in the documentation (or don't know how to use it yet) won't get stuck.

outdated

cbzehner · 2018-02-28T16:15:57Z

I'll update this to remove Duration and provide a hint linking to the documentation.

cbzehner · 2018-03-01T02:38:05Z

exercises/gigasecond/.meta/hints.md

@@ -0,0 +1 @@
+If you're unsure what operations you can perform on `DateTime<Utc>` take a look at the [chrono crate](https://docs.rs/chrono/0.4.0/chrono/) which is listed as a dependency in the `Cargo.toml` file for this exercise.


I wasn't sure if there was a specific format for .meta/hints.md files. I looked at a couple exercism repos on Github but didn't see any. Let me know if this is alright.

There's no specific format necessary there; it's all just human-readable text.

You will, however, need to regenerate the README for this to pass CI:

Get the configlet tool. On mac/linux you can just use the bin/fetch-configlet script; on Windows, you have to find it and install it manually.

Run bin/configlet generate . --only gigasecond to regenerate the readme

Commit the new README.

Thanks for the guidance. I just updated the README.

cbzehner · 2018-03-01T15:39:07Z

exercises/gigasecond/README.md

@@ -4,6 +4,9 @@ Calculate the moment when someone has lived for 10^9 seconds.

 A gigasecond is 10^9 (1,000,000,000) seconds.

+If you're unsure what operations you can perform on `DateTime<Utc>` take a look at the [chrono crate](https://docs.rs/chrono/0.4.0/chrono/) which is listed as a dependency in the `Cargo.toml` file for this exercise.


I'm still a bit concerned this isn't a clear enough hint to walk users to adding Duration as an import from the chrono crate. When I look at the crate documentation on Duration:

Chrono currently uses the time::Duration type from the time crate to represent the magnitude of a time span...

It implies to me as a beginner in Rust that I should be doing the same thing chrono does and adding time to my Cargo.toml so I can also use the time::Duration type.

Duration doesn't appear in reexports and the function most people end up using for this exercise Duration::from isn't apparent anywhere on the page.

Do y'all have any suggestions on how to help this hint provide a bit more structured guidance for people new to the Cargo ecosystem?

I'm still a bit concerned this isn't a clear enough hint to walk users to adding Duration as an import from the chrono crate.

I see what you mean because there's a big dedicated section about durations being from time and being unclear on what relationship this has with https://docs.rs/chrono/0.4.0/chrono/struct.Duration.html.

I don't have any definitive answers, but how about some ideas?

Shall we link to the relevant + operator? https://docs.rs/chrono/0.4.0/chrono/struct.DateTime.html#method.add-1

Shall we link to the current Duration? https://docs.rs/chrono/0.4.0/chrono/struct.Duration.html

coriolinus · 2018-03-01T15:56:45Z

The crate documentation for Duration is actually here <https://docs.rs/chrono/0.4.0/chrono/struct.Duration.html>. The section you linked to is indeed present in the chrono docs, but AFAICT it's stale. In the end, people will just need to experiment. I haven't tried myself, but I believe that attempting to use a time::Duration where a chrono::Duration is expected will just fail with recent versions of chrono. If people try that, and it fails, they'll need to continue looking at the documentation until they find the solution which does work.

…

On Thu, Mar 1, 2018 at 4:39 PM, Chris Zehner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In exercises/gigasecond/README.md <#436 (comment)>: > @@ -4,6 +4,9 @@ Calculate the moment when someone has lived for 10^9 seconds. A gigasecond is 10^9 (1,000,000,000) seconds. +If you're unsure what operations you can perform on `DateTime<Utc>` take a look at the [chrono crate](https://docs.rs/chrono/0.4.0/chrono/) which is listed as a dependency in the `Cargo.toml` file for this exercise. I'm still a bit concerned this isn't a clear enough hint to walk users to adding Duration as an import from the chrono crate. When I look at the [crate documentation on Duration]:(https://docs.rs/ chrono/0.4.0/chrono/#duration) Chrono currently uses the time::Duration type from the time crate to represent the magnitude of a time span... It implies to me as a beginner in Rust that I should be doing the same thing chrono does and adding time to my Cargo.toml so I can also use the time::Duration type. Duration doesn't appear in reexports <https://docs.rs/chrono/0.4.0/chrono/#reexports> and the function most people end up using for this exercise Duration::from isn't apparent anywhere on the page. Do y'all have any suggestions on how to help this hint provide a bit more structured guidance for people new to the Cargo ecosystem? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#436 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTq3VCJuuAtDyvGWJZpMqG56zqgg2ks5taBYcgaJpZM4SSfIL> .

a hint pointing to the chrono crate

cbzehner · 2018-03-01T16:40:42Z

Failed the test due to removing Duration from the example.rs file. Updated to resolve test failures.

coriolinus

Thanks @cbzehner, I think this is in a good state!

I intend to merge this on the 6th, unless further discussion in the interim reveals blocking changes.

coriolinus previously requested changes Feb 25, 2018

View reviewed changes

cbzehner commented Feb 26, 2018

View reviewed changes

cbzehner commented Mar 1, 2018

View reviewed changes

gigasecond: Update dependency to conform to usage guidelines and provide

e027003

a hint pointing to the chrono crate

coriolinus approved these changes Mar 2, 2018

View reviewed changes

ijanos approved these changes Mar 3, 2018

View reviewed changes

coriolinus merged commit 6615150 into exercism:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gigasecond: Update dependency to conform to usage guidelines #436

gigasecond: Update dependency to conform to usage guidelines #436

cbzehner commented Feb 25, 2018

coriolinus left a comment

coriolinus Feb 25, 2018

coriolinus Feb 25, 2018

coriolinus Feb 25, 2018

cbzehner commented Feb 26, 2018

cbzehner Feb 26, 2018

coriolinus Feb 27, 2018

petertseng Feb 27, 2018

ijanos Feb 28, 2018

cbzehner commented Feb 28, 2018

cbzehner Mar 1, 2018

coriolinus Mar 1, 2018

cbzehner Mar 1, 2018

cbzehner Mar 1, 2018 •

edited

Loading

petertseng Mar 4, 2018

coriolinus commented Mar 1, 2018 via email

cbzehner commented Mar 1, 2018

coriolinus left a comment

		@@ -0,0 +1 @@
		If you're unsure what operations you can perform on `DateTime<Utc>` take a look at the [chrono crate](https://docs.rs/chrono/0.4.0/chrono/) which is listed as a dependency in the `Cargo.toml` file for this exercise.

		@@ -4,6 +4,9 @@ Calculate the moment when someone has lived for 10^9 seconds.

		A gigasecond is 10^9 (1,000,000,000) seconds.

		If you're unsure what operations you can perform on `DateTime<Utc>` take a look at the [chrono crate](https://docs.rs/chrono/0.4.0/chrono/) which is listed as a dependency in the `Cargo.toml` file for this exercise.

gigasecond: Update dependency to conform to usage guidelines #436

gigasecond: Update dependency to conform to usage guidelines #436

Conversation

cbzehner commented Feb 25, 2018

coriolinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbzehner commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbzehner commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbzehner Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coriolinus commented Mar 1, 2018 via email

cbzehner commented Mar 1, 2018

coriolinus left a comment

Choose a reason for hiding this comment

cbzehner Mar 1, 2018 •

edited

Loading