Skip to content
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

[Suggestion] Use nonempty library instead of own implementation of OneOrMany #325

Open
1 task done
SeseMueller opened this issue Feb 27, 2025 · 1 comment
Open
1 task done

Comments

@SeseMueller
Copy link

  • I have looked for existing issues (including closed) about this

Feature Request

The struct OneOrMany in this file is used in many places, but is doing the same thing as the well-maintained library nonempty is doing.

Motivation

While the manual implementation is impressive, it is lacking a few things that the nonempty library does provide (Note that both structs look essentially the same). Additionally, errors in the implementation are much more likely to be caught in the library, improving the correctness guarantees somewhat.

Lastly, the library has no forced dependencies, so it wouldn't clutter the cargo file.

Proposal

Replace all instances of the OneOrMany struct with the NonEmpty struct from that library.

Drawbacks

The NonEmpty struct does not provide the specialized string deserialization, so the replacement wouldn't be trivial. But it does have a feature flag to make serialization and deserialization available.

@0xMochan
Copy link
Contributor

0xMochan commented Feb 27, 2025

Hey thanks for this issue! Originally, OneOrMany was introduced in a soft-manner and then expanded upon. I do agree that using a more correct implementation would be a better way for us to go about it. I'll audit that library and see if we can leverage it across out codebase.

There's also some thorns in how we currently use OneOrMany that I'm not a fan of, particularly the .map and .try_map which simplified a ton of code bc you could ensure converting between OneOrMany w/o loosing the 1-item invariant in the iterator (which afaik, isn't representable in the type system in rust due to the lack of higher-kinded types). There's also some other custom impls that might restrict us so I'll have to see what would be ergonomic and such.

Lastly, it would be a breaking change so I want to be sure it's the best path forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants