fix: support qualified types in topic handlers - #344
immanuwell wants to merge 1 commit into
Conversation
Signed-off-by: immanuwell <pchpr.00@list.ru>
mikeee
left a comment
There was a problem hiding this comment.
I'm in favour of this change with a few changes, please could you clarify whether allowing non-stdlib definitions of String should be allowed as implemented?
I think for the string type assertion it'd be better if we asserted against the entire type path, something like this:
fn is_string_type(ty: &Type) -> bool {
let Type::Path(type_path) = ty else {
return false;
};
if type_path.qself.is_some() {
return false;
}
let path = &type_path.path;
let matches_path = |names: &[&str]| {
path.segments.len() == names.len()
&& path
.segments
.iter()
.zip(names)
.all(|(seg, name)| seg.ident == *name && seg.arguments.is_empty())
};
matches_path(&["String"])
|| matches_path(&["std", "string", "String"])
|| matches_path(&["alloc", "string", "String"])
}wdyt?
| .last() | ||
| .is_some_and(|segment| segment.ident == "String"), |
There was a problem hiding this comment.
Was this intentionally added to allow String types defined by any other crates such as other_crate::String?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Thank you for adding tests, please enhance this with negative tests to confirm the above review assumptions.
|
This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 67 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description
#[topic]blows up if the handler arg uses a qualified type likeserde_json::Value.The macro was splitting tokens on
:soserde_json::Valuegot treated like 2 inputs. This switches it to parse the function signature withsyn, keeps theStringfast path, and adds regression tests. tiny fix, but pretty real.Repro:
Before:
cargo checkpanics withExpected to only have one input variableAfter:
cargo checkpasses.Issue reference
This PR will close #172
Checklist