libtime: strftime now returns Result<String, String> instead of String.
#17467
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. |
hatahet
force-pushed
the
mybranch
branch
5 times, most recently
from
83c2077 to
79ec54e
Compare
| @@ -872,7 +871,7 @@ pub fn strptime(s: &str, format: &str) -> Result<Tm, String> { | |||
| } | |||
|
|
|||
| /// Formats the time according to the format string. | |||
| pub fn strftime(format: &str, tm: &Tm) -> String { | |||
| pub fn strftime(format: &str, tm: &Tm) -> Result<String, String> { | |||
There was a problem hiding this comment.
In general the pattern for errors such as this has been Result<T, CustomErrorEnum> to provide more contextual information sometimes and also to be able to programmatically inspect an error result to see what happened. Could you add a custom enum for this method to return? Also be sure to implement Show for the enum to basically return the same String that's already being returned.
Also, can you update the documentation for this function to explain why the return value is a Result, and exactly what cases will cause it to return an Err?
There was a problem hiding this comment.
Sounds good. Could you refer me to other implementations that do what you describe regarding implementing custom error enums?
Also, if I am going to do this, then strptime should change as well to reflect the same I suppose.
There was a problem hiding this comment.
The parsers in semver have some examples of this. It's ok to not update strptime at this time, it can always be done as part of a later PR as well!
There was a problem hiding this comment.
Done. I went ahead an fixed strptime anyway :)
|
@alexcrichton I addressed the review comments. Should I submit a separate commit, or should I merge both commits into one, then update the PR? |
|
A separate commit makes it a bit easier to review. |
hatahet
force-pushed
the
mybranch
branch
2 times, most recently
from
cc8fa93 to
c909bbd
Compare
|
Looks great to me, thanks @hatahet! Could you squash the two commits together and expand the commit message a little more with respect to our breaking changes policy? Just need to add some rationale as to why the API is changing and how to migrate code from the old to the new (should be pretty simple in this case). |
|
@alexcrichton Done! How does the comment look? |
|
Looks great, thanks @hatahet! |
|
It looks like travis found a few tidy errors though, could you fix the lines larger than 100 chars? |
…ing, ParseError>`. `strftime` currently returns a `String`. This does not indicate that this function may return an error due to to a malformed format string. This change introduces a `ParseError` enum which indicates the type of error that occurred. The return type of `strptime` was also changed to use this new enum instead of returning `Result<String, String>`. Now, all instances where `strftime` was used need to have their return value checked to see if it were valid or not. [breaking-change]
|
@alexcrichton Done! |
feat: add bool_to_enum assist for parameters
## Summary
This PR adds parameter support for `bool_to_enum` assists. Essentially, the assist can now transform this:
```rs
fn function($0foo: bool) {
if foo {
println!("foo");
}
}
```
To this,
```rs
#[derive(PartialEq, Eq)]
enum Bool { True, False }
fn function(foo: Bool) {
if foo == Bool::True {
println!("foo");
}
}
```
Thanks to `@/davidbarsky` for the test skeleton (:
Closes rust-lang#17400
- unrelated to the PR but I wanted to change this in rust-lang#17467
- unrelated to the PR but I wanted to change this in rust-lang#17467
[breaking-change]