Add EitherWriteable and use it for DoublePlaceholder in currency format #4750
Conversation
sffc
requested review from
younies
and removed request for
a team and
zbraniecki
utils/writeable/src/lib.rs
Outdated
| /// A [`Writeable`] that is either one type or another type. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[allow(clippy::exhaustive_enums)] // defined to be an enumeration over two types | ||
| pub enum EitherWriteable<W0, W1> { |
There was a problem hiding this comment.
either::Either is the standard Rust type to do this. Can we use that instead of defining our own?
There was a problem hiding this comment.
Fewer deps seems nice... we already have EitherYoke and EitherProvider defined in their respective crates. either isn't a "standard"; it's just a popular crate, like anyhow is a popular error handling crate but isn't a "standard".
@Manishearth opinion?
There was a problem hiding this comment.
I'm okay with an optional dep here. I'm also somewhat open to moving EitherYoke to Either if possible.
a "standard"
"standard Rust type" does not necessarily mean it is "a standard", these have two different implications. I'd say either is the standard Rust type for this.
There was a problem hiding this comment.
In general when it comes to deps we already pull in in a couple places I'm less worried about them. I still want our utils to be simple but it's fine if it's optional deps for crates that are extremely commonly found in Rust.
|
Apparently |
(yeah this is why I am also generally ok with introducing either in ICU4X for the larger deps) |
|
@younies if you have any comments I can fix them in a follow-up. Turns out I need this PR for person names also, so I went ahead and merged it. |
Cleans up the currency formatter code a bit.
Part of #1441