Skip to content
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

Implement Gigasecond exercise #79

Merged
merged 2 commits into from
Mar 21, 2016

Conversation

IanWhitney
Copy link
Contributor

Adding the Gigasecond exercise for the Rust track.

This exercise is a little odd in Rust as the core language has no library for dealing with time. There was the Time crate, but that was pulled from the standard library. Based on this RFC from last year, the Chrono crate appears to be the best current library for dealing with time. I've used Chrono here.

Either because I'm still new to Rust, or because of an actual limitation in Chrono, the best way I could write the tests was to have everything be a UTC DateTime. This means that the tests that only use dates still require the slightly weird .and_hms(0,0,0) function at the end to make them DateTime-y. I did this because Chrono only seems to support Duration addition for DateTimes, not Dates.

@IanWhitney IanWhitney force-pushed the implement_gigasecond branch 2 times, most recently from a2d42a3 to b23792a Compare March 6, 2016 17:19
@IanWhitney
Copy link
Contributor Author

Pinging this one. Is there more work I should do to get this merged?

@kytrinyx
Copy link
Member

/cc @EduardoBautista @petertseng Do either of you have a moment to review this, please?

@EduardoBautista
Copy link
Contributor

I am not too sure of this one. I don't think we should include exercises that require including other libraries. Also, if we do include it, it should probably be at the top since the solution appears to be simple after including all the other libraries.

Thoughts @petertseng ?

@petertseng
Copy link
Member

I'll endeavor to look in no more than 52 hours from now (please remind me if I fail), probably earlier. In the meantime, a few thoughts.

Regarding the question of including a crate - well robot-name which comes only four exercises later has a dependency on the rand crate. Now, we could say that rand is slightly difference since that is a crate that used to be in the standard library and then was moved into a crate. Would we favor to remove robot-name in that case? Or is rand okay because it was once part of the standard library? If the latter, would it be okay to use just time, which was also in the standard library? By just using time::Tm or something similar?

It may be good to get Rust students to learn about Cargo at some point during the track. The Rust book does this as well. On the other hand, I can understand a desire to keep our exercises self-contained and without dependencies.

All this is to say: I'm not sure yet and all I'm doing is listing the advantages and disadvantages I can think of right now. I'll think about it more and try working through this exercise from the point of view of a student and see if any thoughts come up.

@petertseng
Copy link
Member

From the point of view of the student, what they have to do is no different - they still write their code and run cargo test. It's just that doing so also pulls down the libraries. They'll have to include the extern crate and use lines to make that work. I expect most students read the test file so they'll know, but I could be wrong...

So, if we add this exercise, what we want students to take away from it is not anything algorithmically complicated. That's for exercises like rectangles, dominoes, or go-counting (not of this track). What we want them to take away is how to deal with dates/times (This is true of this exercise for all languages. And this is useful for various things) and how to deal with external libraries (This is just true for Rust; a lot of other languages have time/date in their standard library).

Ultimately, I'm not opposed to having this exercise even if it requires the chrono crate. It could be useful to get some light exposure to external libraries and the crates.io ecosystem since that's one of the things the Rust book talks about. I'm open to changing my mind of course if @EduardoBautista or anyone else feels super strongly about not having external crates.

Another interesting thing to know is if having to get the crate causes any student any troubles when doing the exercise. I would think that if anyone has any trouble they will probably file an issue here, comment on their submission, or ask in Exercism support chat. In an interesting future maybe we could ask students for exercise feedback, but I think that's in a big list of features that we want to have, as in exercism/exercism#1081

If there's a future world where time/date handling is in Rust's standard library, then that solves the concern about an external crate. But do we have any indication that that's happening in the near future? If not, I wouldn't wait for something that we don't know will happen.

So if we want to have this exercise, @IanWhitney I think we should include an explanation (for the student to read) about cargo and having a dependency on a crate, maybe link to https://doc.rust-lang.org/book/guessing-game.html#generating-a-secret-number (the current section in the Rust book where they introduce the rand crate).

@IanWhitney
Copy link
Contributor Author

I can add some documentation, no problem.

Adding the [Gigasecond
exercise](http://x.exercism.io/problems/gigasecond) for the Rust track.

This exercise is a little odd in Rust as the core language has no library for
dealing with time. There was the Time crate, but that was pulled from
the standard library. Based on [this
RFC](rust-lang/rfcs#619) from last year, the
Chrono crate appears to be the best current library for dealing with
time. I've used Chrono here.

Either because I'm still new to Rust, or because of an actual limitation
in Chrono, the best way I could write the tests was to have everything
be a UTC DateTime. This means that the tests that only use dates still
require the slightly weird `.and_hms(0,0,0)` function at the end to make
them DateTime-y.

I did this because Chrono only seems to support Duration addition for
DateTimes, not Dates.

I'm following the Ruby track example and putting this exercise after
Hamming.
@IanWhitney IanWhitney force-pushed the implement_gigasecond branch from b23792a to de70629 Compare March 18, 2016 23:01
@petertseng
Copy link
Member

In its current state, this PR has my support. I'm going to wait at least through the weekend to see if @EduardoBautista wants to say something since I'm pretty new to this track so am still getting a sense of what we'd like to do with it. If nothing else, I can merge it, since I'm willing to give the crate thing a try and we're always allowed to change our minds later.

@EduardoBautista
Copy link
Contributor

Oh sorry. Yeah, looks good to me as well. Merging.

EduardoBautista added a commit that referenced this pull request Mar 21, 2016
@EduardoBautista EduardoBautista merged commit d8ae059 into exercism:master Mar 21, 2016
@EduardoBautista
Copy link
Contributor

Thanks @IanWhitney!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants