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

Add Datelike and Timelike example #366

Merged
merged 4 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@CrockAgile
Copy link
Contributor

CrockAgile commented Nov 13, 2017

fixes #357

@budziq
Copy link
Collaborator

budziq left a comment

@CrockAgile Thanks!, The PR is almost perfect!
Just few minor suggestions below.

let now = Utc::now();
// available due to chrono::Timelike
let second = now.second();

This comment has been minimized.

@budziq

budziq Nov 13, 2017

Collaborator

I'd suggest:

  • running rustfmt on the example
  • dropping the intermediate variables and making the queries directly in the println!body
    let (is_pm, hour) = now.hour12();
    println!(
        "The current UTC time is {:02}:{:02}:{:02} {}",
        hour,
        now.minute(),
        now.second(),
        if is_pm { "PM" } else { "AM" },
    );

The example should ges slightly less noisy.

This comment has been minimized.

@CrockAgile

CrockAgile Nov 13, 2017

Author Contributor

I agree. Good change

@@ -1385,6 +1386,44 @@ fn main() {
}
```

[ex-examine-datetime]: #ex-examine-datetime
<a name="ex-examine-datetime"></a>
## Examine a DateTime struct via Datelike and Timelike traits

This comment has been minimized.

@budziq

budziq Nov 13, 2017

Collaborator

I'd probably avoid using the tpe and trait names in the example title. How about something along the lines of "Examine the date and time"?

This comment has been minimized.

@CrockAgile

CrockAgile Nov 13, 2017

Author Contributor

Sounds good! I was trying to be explicit toward the issue, but you are right that including it makes it a little verbose/wordy. Thanks for the review

CrockAgile added some commits Nov 13, 2017

@ludwigpacifici

This comment has been minimized.

Copy link
Contributor

ludwigpacifici commented Nov 13, 2017

Will it be useful to show also the other way around, i.e. from a string date (example "2017/12/31T12:34:56-1:00") get the DateTime via parse_from_rfc3339 or parse_from_str?

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Nov 13, 2017

Will it be useful to show also the other way around, i.e. from a string date

We already have date parsing example I'd suggest to keep this example focused as it is.

fn main() {
let now = Utc::now();
// available due to chrono::Timelike

This comment has been minimized.

@ludwigpacifici

ludwigpacifici Nov 13, 2017

Contributor

The comment repeats what is said in recipe description (few lines above). Does it make sense to remove it (to make example slightly shorter)?

now.num_seconds_from_midnight()
);
// available due to chrono::Datelike

This comment has been minimized.

@ludwigpacifici

ludwigpacifici Nov 13, 2017

Contributor

The comment repeats what is said in recipe description (few lines above). Does it make sense to remove it (to make example slightly shorter)?

This comment has been minimized.

@budziq

budziq Nov 13, 2017

Collaborator

Yeah, this is a good idea.
Lets drop the comments and we are ready to merge!

Also, the example title is still a little off but I can't come up with a better one at the moment. @CrockAgile If you can come up with something slightly more natural please propose one but it will not block the PR.
Well done!

This comment has been minimized.

@CrockAgile

CrockAgile Nov 13, 2017

Author Contributor

@budziq I also can't get a better title in my head, but I agree it is lacking. "Get current date and time"? I am open to suggestions. Hard to be more detailed/exciting when there are already examples for parsing and modifying datetimes.

@ludwigpacifici

This comment has been minimized.

Copy link
Contributor

ludwigpacifici commented Nov 13, 2017

We already have date parsing example I'd suggest to keep this example focused as it is.

My bad, I have not seen it. Thanks!

@budziq budziq merged commit eec1522 into rust-lang-nursery:master Nov 13, 2017

1 of 2 checks passed

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

This comment has been minimized.

Copy link
Collaborator

budziq commented Nov 13, 2017

Thanks @CrockAgile!

Please drop a comment on #209 if you consent to repository being relicensed with CC0 license!

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.