Possible fix #47 Add flag Option for bool Flag#70
Possible fix #47 Add flag Option for bool Flag#70GREX18 wants to merge 1 commit intofunnyboy-roks:mainfrom
Conversation
funnyboy-roks
left a comment
funnyboy-roks
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at this! I don't think there were any changes made for the type-state builder, so that will need to be done. Apart from that, I have just a few comments!
| if self.attr.flag { | ||
| let attributes = &self.attr.attributes; | ||
| let konst = builder_attr.konst_kw(); | ||
| return quote! { #(#attributes)* #[must_use = "You have created a flag type but not used it"] #builder_vis #konst fn #fn_ident(#self_param) -> #return_type { #[allow(deprecated)] { self.#inner.#field_i = Some(true); } self } }; } |
There was a problem hiding this comment.
This should not need to be duplicated from below. You should be able to just change the setter variable above and change the to_args_and_value function on FieldAttr to have no arguments.
| } | ||
| // ensure it's a boolean attribute | ||
| if !matches!(&field.ty, Type::Path(tp) if tp.path.is_ident("bool")) { | ||
| bail!(ident.span() => "`flag` must affect a boolean attribute"); |
There was a problem hiding this comment.
Perhaps reword this to something like
| bail!(ident.span() => "`flag` must affect a boolean attribute"); | |
| bail!(ident.span() => "`flag` may only be applied to a boolean field"); |
| } else if field.attr.flag { | ||
| quote! { | ||
| inner.#field_i.take().unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
I would prefer the field on the builder hold a bool, rather than need to unwrap this option.
| } else if let Some(default) = &field.attr.default { | ||
| let default = default.to_value(field.attr.into); | ||
| quote! { | ||
| // NOTE: not using Option::unwrap_or_else, since it's not stable in const |
There was a problem hiding this comment.
Why remove this comment (and the one below)?
There was a problem hiding this comment.
Use the same macro pattern from the simple test in this one, so that all builder kinds are tested.
There was a problem hiding this comment.
These changes should not be included in here. The messages depend on what compiler you use and we're currently testing against stable.
2 participants
This should fix issue #47 by adding the flag option as well as some simple tests that could be expanded.
.unwrap doesn't work with it unfortunatley