-
Notifications
You must be signed in to change notification settings - Fork 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
Implement FromIterator
for OneOrMany
#602
Conversation
Does |
Yes, it is possible to do that. EDIT: |
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.
Looks good to me.
The name of the type implies that it has len > 0
so perhaps it makes sense to enforce that in a future PR, if it currently allows invalid deserializations.
Note that it also has an In the future we can definitely consider at least making |
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.
Looks good to me! Thanks for the tests.
I don't think we use the Chore
label for PRs anymore, just issues.
I think restricting OneOrMany
to reject empty collections would be quite a large refactor since it would require bubbling-up errors and replacing From
with TryFrom
in quite a few places. I also think we need to keep a custom implementation rather than replacing it with NonEmpty
for instance as the special serialization for the One
variant is important.
OneOrMany
FromIterator
for OneOrMany
Description of change
This PR consists of a couple of small improvements to
OneOrMany
:OneOrMany<T>
now implementsFromIterator<T>
push
method now behaves arguably more intuitively in the edge case ofOneOrMany::Many(vec![])
.To elaborate further on the latter. The following test did not pass before this PR:
This is not a bug, but most would probably expect the
One
variant to be matched when there is exactly one element in the collection. Our change should not affect the behaviour in any other cases.Links to any relevant issues
Be sure to reference any related issues by adding
fixes issue #
.Type of change
Add an
x
to the boxes that are relevant to your changes.How the change has been tested
Tested locally with
cargo test
.Change checklist
Add an
x
to the boxes that are relevant to your changes.