feat: add 'required' attribute for Option fields#58
feat: add 'required' attribute for Option fields#58Deepak8858 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 tackling this! I am glad to see it really was just changing how wrapped_option is set!
There are a few notes, but nothing too major. Your branch is somewhat out dated, but most of the changes I've made since then are just on restructuring so updating should be pretty straightforward.
Especially since I'm pretty heavily developing this crate right now, if you want to pick up other issues, leave a note on the issue that you're working on so I don't also start working on it 😅
| impl Attribute { | ||
| fn as_str(self) -> &'static str { | ||
| self.into() | ||
| const ALL: [Self; 10] = [ | ||
| Self::Default, | ||
| Self::Into, | ||
| Self::Repeat, | ||
| Self::RepeatN, | ||
| Self::Rename, | ||
| Self::SkipPrefix, | ||
| Self::SkipSuffix, | ||
| Self::Tuple, | ||
| Self::Adapter, | ||
| Self::Required, | ||
| ]; | ||
|
|
||
| const fn as_str(self) -> &'static str { | ||
| match self { | ||
| Attribute::Default => "default", | ||
| Attribute::Into => "into", | ||
| Attribute::Repeat => "repeat", | ||
| Attribute::RepeatN => "repeat_n", | ||
| Attribute::Rename => "rename", | ||
| Attribute::SkipPrefix => "skip_prefix", | ||
| Attribute::SkipSuffix => "skip_suffix", | ||
| Attribute::Tuple => "tuple", | ||
| Attribute::Adapter => "adapter", | ||
| Attribute::Required => "required", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In #49, this was modified to be less manual. Those changes will appear when you update your branch.
| if out.required { | ||
| bail!(ident.span() => "`required` may only be used once."); | ||
| } | ||
| out.required = true; |
There was a problem hiding this comment.
Add a check here to ensure that the the type is Option. (I'd probably just re-use get_single_generic)
| /// Force a field to be specified, even if it is an `Option`. Normally, `Option` fields are | ||
| /// optional. |
There was a problem hiding this comment.
I think it would be better to have this be a bit more concise, something like
Force an optional field to be specified using
Option
I think the examples do most of the work for this attribute :)
| use bauer::Builder; | ||
|
|
||
| #[test] | ||
| fn required_option_owned() { |
There was a problem hiding this comment.
Can you add an instance of this test for the borrowed kind
|
|
||
| let err = Foo::builder().build().unwrap_err(); | ||
| // The exact error name depends on the field name | ||
| assert!(format!("{:?}", err).contains("MissingField")); |
There was a problem hiding this comment.
You should be able to do a proper equality comparison here:
| assert!(format!("{:?}", err).contains("MissingField")); | |
| assert_eq!(err, FooBuildError::MissingField); |
3 participants
Added a
requiredattribute forOptionfields that are normally unwrapped to be optional. This allows forcing a field to be specified even if its type isOption<T>. This works for both default (owned/borrowed) and type-state builders. Fixes #55.