Allow actions to return ActionResult values to represent errors #76
Conversation
This breaks all clients.
(the original action handling is still broken)
src/app.rs
Outdated
| self.help_text(), | ||
| )) { | ||
| Ok(_) => (), | ||
| Err(e) => panic!("{}", e.message), |
There was a problem hiding this comment.
@ksk001100 Instead of calling panic! here, I think it would be better to return an error from this run function, which we might rename to run_with_result. To make this a nonbreaking change, we could add a new run function that has the same interface as the current one and which calls run_with_result and panics if run_with_result returns an error. What do you think about this? If it sounds good to you, I could make that change in this PR or a followup (please let me know your preference).
For background, my overall goal is to remove calls to panic! from command execution, generally following this discussion. Thanks!
There was a problem hiding this comment.
@timburks
I think that suggestion is good, please implement run_with_result.
There was a problem hiding this comment.
We would like to replace the run implementation with a run_with_result implementation in the future, as we want to prioritize simplicity.
There was a problem hiding this comment.
@ksk001100 Thanks for your feedback! Yes, I'll proceed with that in this PR.
There was a problem hiding this comment.
@ksk001100 I think run_with_result is ready to review. I've added some top-level tests that demonstrate calling it and I've put a comment in #75 showing how it can be used to return Result values directly from main().
I agree with eventually fully replacing run with run_with_result (and just calling the new function run), and also agree with waiting for a major version bump to make that breaking change. I'm happy to make those changes whenever you'd like to do that.
Since this is a fairly large change, feel free to take your time to review and get feedback from anyone else before merging. Thank you!
|
@timburks |
Fixes #75
This PR contains a nonbreaking change that adds support for a new action field,
action_with_result, that can be specified instead ofactionforCommandandAppstructs. These fields take functions that return a newResulttype that includes an error value that can be checked in the Seahorse calling code.done (ready to verify):
ActionWithResulttypes and handling.actionhandling.actionandaction_with_result.todo: