feat: Add extractor hooks and validation system #781
Conversation
|
Hey Josh. I would love to hear your take on how I implemented this feature. If it can be done better, please let me know |
- Add ExtractorHook trait for lifecycle observability - Add ExtractorValidatorHook trait for custom validation - Add ExtractorWithHooks extension trait - Add comprehensive documentation and examples - Add integration tests for hook functionality"
Soroushsrd
force-pushed
the
validator
branch
from
5185b8f to
a496118
Compare
There was a problem hiding this comment.
Great work! Some changes needed but I think otherwise mostly OK. Good job on ensuring docstrings, too.
Generally speaking:
- Avoid
Vec<Box<T>>(and&[Box<T>]) where possible as an input argument - we should be handling this internally (seeToolSetstruct if stuck). For example:
struct Hooks(Vec<Box<dyn ExtractorHook>>);
impl Hooks {
fn add_hook<T>(mut self, hook: T) -> Self
where
T: ExtractorHook + 'static {
self.0.push(Box::new(hook));
self
}
}
- Having
ExtractorWithHooks<T>might be a bit too far of a leap in terms of traits at the moment. Ideally we should have a way to simply transition from one struct,Extractor<M, T>to anExtractorWithHooks<M, T>. - Please add an example to the
rig-coreexamples list.
| /// ``` | ||
| /// | ||
| /// [`extract_with_hooks`]: ExtractorWithHooks::extract_with_hooks | ||
| pub trait ExtractorWithHooks<T> |
There was a problem hiding this comment.
I think this is generalising perhaps a bit too far for the scope. We should ideally have an ExtractorWithHooks struct I think for now. If it's evident that users actually want this trait because they need to build their own extractors (and the extractor hooks into other things), then I think we should consider it then.
| /// validation failure will cause the extraction attempt to fail: | ||
| /// | ||
| /// ```rust | ||
| /// let validators: Vec<Box<dyn ExtractorValidatorHook<Person>>> = vec![ |
There was a problem hiding this comment.
We should try to abstract away as much as possible the need to use Vec<Box<T>> from the user side. It is a bit clunky in terms of DX.
|
|
||
| for i in 0..=self.retries { | ||
| tracing::debug!( | ||
| "Attempting to extract Json. Retries left:{retries}", |
There was a problem hiding this comment.
should be JSON or json, not Json
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Attempt number {i} to extract Json failed: {e:?}. Retrying..." |
| /// This implementation creates the complete extraction lifecycle with | ||
| /// observability and validation. It manages the retry loop, coordinates | ||
| /// hook calls, and ensures validators are executed in the correct sequence. | ||
| fn extract_with_hooks( |
There was a problem hiding this comment.
fyi I believe when you are implementing a trait for a type (that has fn() -> <some_future>, you can use async fn rather than just rawdogging the future
| &self, | ||
| text: impl Into<Message> + Send, | ||
| attempt: u64, | ||
| hooks: &[Box<dyn ExtractorHook>], |
There was a problem hiding this comment.
as mentioned before, we should try to avoid using arrays of Box<T> where possible
|
|
||
| #[tokio::test] | ||
| #[ignore] | ||
| async fn test_multiple_validators() { |
There was a problem hiding this comment.
general comment: An example to hold one of these tests in would be great. I know there's literally a gazillion examples already (and we will be cleaning it up!), but for now an example would be great.
|
Got it. thank you for the comments. will sort them out. |
closes #770