elabit / robotmk

Robotmk - the Robot Framework integration for Checkmk
https://robotmk.org
GNU General Public License v2.0
54 stars 10 forks source link

Dev/sol/interrupt #574

Closed SoloJacobs closed 3 months ago

SoloJacobs commented 3 months ago

Not complete, but it makes sense to review the changes up to this point.

jherbel commented 3 months ago

For my understanding: For example, the commit "Remove exit-code if shutting down during GeneralSetup" does not actually remove the exit code on its own, right? Only in combination with the changes to main.rs in "Remove exit-code 1 if shutting down during write" the exit code is removed, right?

SoloJacobs commented 3 months ago

@RobWalt I hope you don't mind me asking a question. Don't feel obliged to answer.

We realized, that terminating our program with SIGINT/systemd would always result in a failed state for systemd. For this reason there are usually three outcomes for each function: Ok(ComputationOutcome), Err(TeminateWithError), Err(TerminateWithoutError).

This is reflected in the changes in this pull request. However, every function in our code now has it's own TerminateWithoutError type, since enum variants cannot be shared between different enums. Alternatively, we could have every function have the same Error types, but that means some functions will have errors in their return signature, which they don't actually produce. (This is the same as the std library chooses with std::io::Error).

With pythons duck typing this can been solved differently:

class TerminateWithError: pass

class TerminateWithoutError: pass

def setup() -> TerminateWithError | TerminateWithoutError | Plans: 
   ...

def rcc_setup() -> TerminateWithError |TerminateWithoutError | RccPlans:
   ...

How does one approach such a problem idiomatically in rust?

RobWalt commented 3 months ago

@SoloJacobs no worries!

I'm not entirely sure what exactly your pain points are. From looking at your code, you have a lot of repetitive error types now. What came to mind was that you might be able to construct a generic error of the same form and implement the chore-y parts only once. E.g. something like

Code ```rust use std::marker::PhantomData; use thiserror::Error; #[derive(Debug)] pub struct Write; #[derive(Debug)] pub struct Build; #[derive(Debug)] pub struct Run; #[derive(Debug)] pub struct Lock; // type WriteError = GenericError; // type BuildError = GenericError; // type RunError = GenericError; type LockError = GenericError; fn main() { let e = LockError::new_other(LockVariants::Foo(String::from("Hello, World!"))); println!("{e}"); let e = e.cast_error::(); println!("{e}"); let e = e.cast_error::(); println!("{e}"); let e = e.cast_error::(); println!("{e}"); } #[derive(Error, Debug)] pub enum ErrorVariant { #[error(transparent)] Other(E), #[error("Terminated")] Cancelled, } #[derive(Error, Debug)] #[error("Failed to `{0}`")] pub struct Unrecoverable(String); #[derive(Error, Debug)] pub enum LockVariants { #[error("Failed to open `{0}`")] Foo(String), #[error("Could not complete task")] Bar(String), } impl From for Unrecoverable { fn from(value: LockVariants) -> Self { Self(format!("{value:?}")) } } #[derive(Error, Debug)] #[error("{variant}")] pub struct GenericError { variant: ErrorVariant, _pd: PhantomData, } impl GenericError { pub fn new_other(variant: E) -> Self { Self { variant: ErrorVariant::Other(variant), _pd: PhantomData, } } pub fn new_cancelled() -> Self { Self { variant: ErrorVariant::Cancelled, _pd: PhantomData, } } } // instead of `From`, since impl `From` is impossible due to existing impls in the std lib impl GenericError { pub fn cast_error>( self, ) -> GenericError { let variant = match self.variant { ErrorVariant::Other(v) => ErrorVariant::Other(EB::from(v)), ErrorVariant::Cancelled => ErrorVariant::Cancelled, }; GenericError { variant, _pd: PhantomData, } } } ```

Other than that there are ideas to use traits similar to std::error::Error, which you already mentioned. Maybe it's worth taking a look at how big ecosystem libraries handle that problem as well. I'd guess that especially networking crates like hyper would have to solve something similar due to the variety in HTTP response codes.

SoloJacobs commented 2 months ago

@RobWalt Thanks, that was a very helpful. We have found an improvement now :+1: https://github.com/elabit/robotmk/pull/580

I guess in the end just sharing one Error implementation across different functions appears to be the rust way (both std::io::Error and hyper::Error work that way). It also works pretty well for our use case.

RobWalt commented 2 months ago

@SoloJacobs Glad to hear that it helped! :) 🎉