-
Notifications
You must be signed in to change notification settings - Fork 33
Allow external tracing procedures #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
While this would work for me, I rejected the same approach in my PR because it (1) leaves the door open for an inconsistent registry (missing variants) and (2) misses the opportunity to detect an infinite loop due to logic errors that the other approach could detect. The latter is merely a side effect as there is no known case of such an infinite loop AFAICT. The first one is the original argument. See below for a unittest showing this problem.
| Ok((format, value)) | ||
| } | ||
|
|
||
| /// Read the status of an enum and reset the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this you can do this (the reduced test from #73):
#[test]
fn test_pending_enum() {
#[derive(Deserialize)]
enum E {
A,
B,
}
let mut tracer = Tracer::new(TracerConfig::default());
let samples = Samples::new();
let mut format = Format::unknown();
let deserializer = Deserializer::new(&mut tracer, &samples, &mut format);
E::deserialize(deserializer).unwrap();
format.reduce();
let Format::TypeName(name) = &format else {
panic!()
};
assert!(tracer.check_incomplete_enum(name).is_some());
assert_eq!(
tracer.registry(),
Err(Error::MissingVariants(vec!["E".to_owned()]))
);
}which results in:
---- test_pending_enum stdout ----
thread 'test_pending_enum' panicked at serde-reflection/tests/serde.rs:527:5:
assertion `left == right` failed
left: Ok({"E": Enum({4294967294: Named { name: "A", value: Unit }})})
right: Err(MissingVariants(["E"]))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ma2bd/serde-reflection@external_tracing...quartiq:serde-reflection:external_tracing If you want to play with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with this PR and see if we need anything more later.
* upstream/main: Allow external tracing procedures (zefchain#85)
Summary
Following suggestions from @jordens in #73, we make a number of APIs and types
pub.Test Plan
CI