Use a consistent pattern for error handling #67
Comments
iffyio
added
good first issue
|
Given that we're also planning on allowing people to use this as a library - it seems like we should have our own Error enum(s), and expose those as public, rather than expose the underlying libraries to the outside world (especially if we ever want those to change). (To be honest, I only recently started to look more at error handling, and getting more of a handle on it) I'd be inclined to handcraft our of own enums/structs to start, and if we get more complicated, bring in a more sophisticated library as needed. WDYT? |
We needed validation that endpoint names are unique, so implemented a validation function for config. This had a few ripple on effects: - impl block for Config, so made sense to pull `from_reader` into that as a static method. - Needed concrete Error types (impacts #67) for validation and other potential error conditions. `from_reader` will now call validate() to ensure that the struct is valid before completing. `validate` was also left as public, since Config's can be manually created before passing them into `Server.run()`
|
+1 on writing our own structs, it makes sense to introduce a library if/when we know it adds significant value. |
* Add Validation to Config We needed validation that endpoint names are unique, so implemented a validation function for config. This had a few ripple on effects: - impl block for Config, so made sense to pull `from_reader` into that as a static method. - Needed concrete Error types (impacts #67) for validation and other potential error conditions. `from_reader` will now call validate() to ensure that the struct is valid before completing. `validate` was also left as public, since Config's can be manually created before passing them into `Server.run()`
|
I feel like we've resolved this for now - what do you think, can we close this? |
|
We currently have some places where we needed to return proper errors, e.g in server.rs we still pass tokio result - the idea was to resolve those cases in this ticket. |
Ah yes! I had forgotten! Excellent point. |
|
Just did a quick review of server.rs and session.rs -- I think we are good to close this now. One thing I did note - we probably should create a ticket to review of public methods, and restrict them to only being available to the crate or smaller. Because we do still expose How does that sound? |
|
I can create a PR to replace the tokio::io::Result usages and link that to close this PR :+1: |
* Remove remaining returns of Tokio Result Fixes #67 * Use errors file and rename InternalError Co-authored-by: Mark Mandel <markmandel@google.com>
Currently we use mixtures of
tokio::Resultandstd::result::Result<_, std::io::Error>etc when returning errors which isn't ideal.We want to pick a single way to handle errors in the codebase by e.g leveraging an existing library, handcrafting structs ourselves etc.
The text was updated successfully, but these errors were encountered: