Skip to content

Handle any errors #26

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 14 commits into from
Closed

Handle any errors #26

wants to merge 14 commits into from

Conversation

sapessi
Copy link
Contributor

@sapessi sapessi commented Dec 3, 2018

Issue #, if available: #23

Description of changes:
Beginning to address generic error handling in the runtime. Changed the handler to return a Box<dyn Error>. This is still a work in progress.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sapessi sapessi requested a review from davidbarsky December 3, 2018 18:05
@@ -11,8 +11,10 @@
//! extern crate serde_derive;
//! #[macro_use]
//! extern crate lambda_runtime;
//! #[macro_use]
//! extern crate simple_error;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how many people use simple_error. Should we opt for Failure for popularity reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our conversation, I'm keeping the simple error crate in the docs example for simplicity's sake. I've removed the #[macro_use] and added an example specific to the failure crate

extern crate log;
extern crate serde_derive;
extern crate simple_logger;
#[macro_use]
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to have #[macro_use] in Rust 1.30 and above—they can be imported like normal macros. Given that the 2018 edition is coming out on Thursday, I think it's a good idea to be forward-facing with regard to these imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed all over the place.

/// The `HandlerError` struct can be use to abstract any `Err` of the handler method `Result`.
/// The `HandlerError` object can be generated `From` any object that supports `Display`,
/// `Send, `Sync`, and `Debug`. This allows handler functions to return any error using
/// the `?` syntax. For example `let _age_num: u8 = e.age.parse()?;` will return the
Copy link
Contributor

Choose a reason for hiding this comment

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

since this comment is likely going to be read from the perspective of someone reading the rustdoc without much context for the example this is lifted from in the repo, you may just want to give a more isolated example like "not a u8".parse::<u8>()?

@softprops
Copy link
Contributor

softprops commented Dec 17, 2018

wanted to give you a quick heads up about a pull that may conflict with this but it seems there are a number of merge conflicts already present. In any case, in #50, I'm proposing moving away from stringly values for ErrorResponse#errorType

@sapessi
Copy link
Contributor Author

sapessi commented Jan 24, 2019

Closing this PR in favor of the refactor in #63

@sapessi sapessi closed this Jan 24, 2019
@sapessi sapessi deleted the errors branch January 24, 2019 22:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants