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

Extended Try RFC #211

Closed
wants to merge 1 commit into from
Closed

Extended Try RFC #211

wants to merge 1 commit into from

Conversation

TyOverby
Copy link

@lilyball
Copy link
Contributor

Have you read RFC PR #201?

@TyOverby
Copy link
Author

Yeah, I saw that RFC and thought that the hidden layer of indirection isn't worth the lack of verbosity. Since they are both targeting the same problem, I'll close this one if #201 gets accepted.

@bill-myers
Copy link

I don't think this is in conflict with #201.

Rather, it allows to handle cases when you want to wrap an error of the same type in multiple different ways, or in a non-default way, while #201 handles the default case automatically.

Both RFCs seem good to have.

Example:

enum MyError {
 MetadataIoFailed(IoError),
 DataIoFailed(IoError)
}

fn read_data_and_metadata(filename: &str) -> MyError {
 let index = try!(File::open(metadata_filename(filename)), MetadataIoFailed);
 let file = try!(File::open(filename), DataIoFailed);
 // do things
}

@lilyball
Copy link
Contributor

@bill-myers Hrm, that's actually a good point. I was thinking that #201 sufficed, because the resulting wrapper error type is used to figure out how to handle the underlying error, but that doesn't work for your example where MyError has two cases that both wrap an IoError.

@pnkfelix
Copy link
Member

@TyOverby is the optional second argument restricted to always be an identifier? Or can it be an arbitrary expression (presumably any subtype of |: input: InputError| -> OutputError) ?

@TyOverby
Copy link
Author

@pnkfelix : the idea would be that any element that is callable and typechecks would work. My macro-writing skills aren't up to par, but something like that should be feasible right? I will add this detail to the RFC later today; thanks for bringing it to my attention.

@pnkfelix
Copy link
Member

@TyOverby it should be easy to write the appropriate macro (they can be declared as taking an expression or an identifier, among other syntactic classes); I just wanted to double-check about how general you expected this to be.

# Alternatives

* Create another macro that is very similar
* (named `try_or!` ?).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the try! macros is widely used and mostly liked, creating a new macro seems like it would be a much better idea. In that case, I would recommend implementing it as an external library and getting some use and experience, then submitting a PR to add it to std (I don't think that would require an RFC), and perhaps finally an RFC to add it to the prelude if it is very popular.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nick29581 The existing try!() can easily be extended to take two arguments without breaking any backwards compatibility. The suggestion for a new macro is only as a potential readability aid. Personally, I think using a new macro is unnecessary, providing a second arg to try!() should be visible enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its also a good way to get some experience using this without affecting a really crucial macro. I mean that if we added this, decided it was a mistake, then took it out again, it could be painful. So there is a high bar for acceptance. If we had some use of this beforehand to convince us it was a good idea, then it would be easier to accept.

So, change the last sentence in my previous comment to "submit an RFC to merge the new macro in with Try!"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted this macro bad enough that I made my own cargo project that implements this RFC.
https://github.com/TyOverby/try_or

I'm pretty bad at writing macros, so I wouldn't suggest that this be made the official implementation, but I've found that it is incredibly handy.

@TyOverby
Copy link
Author

TyOverby commented Sep 2, 2014

I just thought of another use of this macro: side-effectful error handling.

Imagine the following code:

enum MyError {
 MetadataIoFailed(IoError),
 DataIoFailed(IoError)
}

fn metadata_log(ioe: IoError) -> MetadataIoFailed {
    debug!("failed to get metadata with {}", ioe);
    MetadataIoFailed(ioe)
}

fn dataio_log(ioe: IoError) -> DataIoFailed {
    error!("data io failed with {}", ioe);
    DataIoFailed(ioe)
}

fn read_data_and_metadata(filename: &str) -> MyError {
 let index = try!(File::open(metadata_filename(filename)), metadata_log);
 let file = try!(File::open(filename), dataio_log);
 // do things
}

In this example, the failure in the try branch is logged before being transformed.

@reem
Copy link

reem commented Sep 2, 2014

I might be missing something obvious, but how is this better than just using the existing try! with map_err?

The previous example is just:

enum MyError {
 MetadataIoFailed(IoError),
 DataIoFailed(IoError)
}

fn metadata_log(ioe: IoError) -> MetadataIoFailed {
    debug!("failed to get metadata with {}", ioe);
    MetadataIoFailed(ioe)
}

fn dataio_log(ioe: IoError) -> DataIoFailed {
    error!("data io failed with {}", ioe);
    DataIoFailed(ioe)
}

fn read_data_and_metadata(filename: &str) -> MyError {
 let index = try!(File::open(metadata_filename(filename)).map_err(metadata_log));
 let file = try!(File::open(filename).map_err(dataio_log));
 // do things
}

@brson
Copy link
Contributor

brson commented Sep 5, 2014

Thank you for the RFC @TyOverby. At this time we're more inclined to go in the direction specified in #220, which is a fairly comprehensive overhaul of error handling that includes this functionality. In the meantime, this macro can be implemented by hand (with another name) for those that need the functionality.

Closing.

@brson brson closed this Sep 5, 2014
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
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.

7 participants