internal: internal cleanup; correctness #214
Conversation
| //! } | ||
| //! ``` | ||
| pub use crate::types::LambdaCtx; | ||
| use anyhow::Error; |
There was a problem hiding this comment.
I'm a fan of anyhow but do you think exposing this is the outward facing api is going to last?
I asking because I feel like this is a trap the use error crates in libraries in the rust ecosystem in the past. On year is error_chain, the next year it's failure, the next year it's anyhow. I think these are all really good for use in applications but for library crates it's problematic when the community shifts its opinions forcing applications to deal with breaking library changes when that happens.
There was a problem hiding this comment.
Yeah, it shouldn't be exposed, I'm just very lazy! At the very most, we should depend on thiserror, which was designed to be safely used by libraries by just expanding to std::error::Error with no trace of thiserror. A future removal of thiserror will be equivalent to hand-written std::error::Error implementation.
Now that I think of it, I think that I can write all of the errors using thiserror, but only publish the slightly cleaned up cargo expand output.
There was a problem hiding this comment.
+1 we've been down this error path before. We should try and expose only std errors without forcing an opinion/dependency on function developers
There was a problem hiding this comment.
Yeah, I'll drop it in the next PR and use std::error::Error without any derives. I primarily want to merge this as is so that some other folks can build on these changes more easily.
| } | ||
| fn incoming(client: &Client) -> impl Stream<Item = Result<http::Response<hyper::Body>, Error>> + '_ { | ||
| gen!({ | ||
| let req = NextEventRequest.into_req().expect("Unable to construct request"); |
| //! } | ||
| //! ``` | ||
| pub use crate::types::LambdaCtx; | ||
| use anyhow::Error; |
There was a problem hiding this comment.
+1 we've been down this error path before. We should try and expose only std errors without forcing an opinion/dependency on function developers
3 participants
Issue #, if available: N/A
Description of changes:
This branch has a few changes:
&selfinstead of a&mut self, reducing how many.clone()s we need need to write. This also means that the client no longer implementstower::Service, but given that functionality wasn't used anyways, I don't consider this to much of a loss.next_eventcall, not the response handler. This allows for races to be cleanly-side stepped.Still upcoming:
tracingsupport for logs, metrics, and distributed tracing (e.g., AWS X-Ray Integration).By submitting this pull request