-
Notifications
You must be signed in to change notification settings - Fork 52
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
Field dependencies #96
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.
I started reviewing it without noticing it's a draft, and only realized it's incomplete when I couldn't find where the implementations
are actually used. My bad.
@@ -116,6 +121,7 @@ pub struct FieldBuilderAttr<'a> { | |||
pub default: Option<syn::Expr>, | |||
pub deprecated: Option<&'a syn::Attribute>, | |||
pub setter: SetterSettings, | |||
pub implementations: Option<Vec<Vec<sus_impls::Dependency>>>, |
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.
Nit: I see that you have a pub type Implementation = Vec<Dependency>;
in sus-impls. Wouldn't it be better to use it here?
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.
Also - does it have to be an Option
? If no requires
setting it given, why not generate a single implementation where all the fields are Generic
expect for the current fields which would be Unset
?
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.
I did indeed use the type Implementation
but I did not push the changes yet.
I wanted to avoid allocating vectors in case that the requires
attribute was not used. But I will find out if I should accept that tradeoff to simplify generating methods when I do use implementations
.
"requires" => { | ||
let Some((field_names, ordinal)) = field_names_with_current_orinal | ||
else { | ||
return Err(Error::new_spanned(assign, "`requires` is only allowed as a field attribute, not as a struct attribute.")); |
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.
I find this error message a bit confusing. I think something like "`requires` is not allowed in `field_defaults`" would better convey the meaning.
_ => return Err(Error::new_spanned(expr, "attribute only allows str values")), | ||
}, | ||
syn::Expr::Lit(syn::ExprLit { | ||
lit: syn::Lit::Str(str), .. |
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.
I'm kind of surprised rustfmt okayed it...
BTW, regarding sus-impls - since it's supposed to be a generic crate, wouldn't it be better to have it work with field ordinals instead of |
Closes #14