In 33fb56f:

Can you reimagine this so that it just assigns every variant a number and then we directly compare those? I doubt there will be any measurable performance impact and it will make it much clearer that the resulting order is correct and consistent.

Yeah I thought of doing that but I thought then that every reader of the enum would be like "WTF, why do these variants have this weird non-consecutive value assigned to them. Oh for <, who cares about < for an enum. These guys are off their heads"

Nobody is going to think "who cares about < for an enum". And I'm not talking about giving the variants canonical numeric values. Only giving them values within the cmp function, which is currently very hard to read.

There is already a number assignment for each denomination in precision() which can be used for the comparison:

impl PartialEq for Denomination {
    fn eq(&self, other: &Self) -> bool {
        self.precision() == other.precision()
    }
}

impl Ord for Denomination {
    fn cmp(&self, other: &Self) -> Ordering {
        other.precision().cmp(&self.precision())
    }
}

impl PartialOrd for Denomination {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

The above gives Bit == MicroBitcoin, if that is wanted.

The above gives Bit == MicroBitcoin, if that is wanted.

We don't... in this case we want Bit < MicroBitcoin on the basis that we "fall back to alphabetical order" in case of equal units. But I like the idea of using precision otherwise. Maybe we just want to special-case this pair?

Oh, crap. I see the problem. Ideally we would have BIT as associated const but that prevents us from displaying it.

I know this will be unpopular but I suspect having Eq (and consequently Ord) at all is highly misleading.

I suggest dropping the impls entirely. We can still have methods is_same_magnitude and is_displayed_same_as for equality.

Honestly I'd rather drop MicroBitcoin entirely than drop the Eq impl. In some other issue Tobin was just commenting how weird uBTC is, when it really should be µ, and anyway "micro" is not a SI prefix that most people are very familiar with.

Are both these units used in practice?

"bit" feels more like slang while "microbitcoin" feels "formal" to me. But that might be alright if people prefer bit.

Strictly speaking, we don't need them both since people can write their own enum and manually implement displaying.

I was also thinking of having DisplayDenomination enum with different semantic and hopefully name that suggests the purpose reasonably but it seems too much. Or just rename Denomination to DisplayDenomination and don't add a new one.

Strictly speaking, we don't need them both since people can write their own enum and manually implement displaying.

Yes, except that this was a bug-ridden slog for us and nobody else wants to redo that work.

I was also thinking of having DisplayDenomination enum with different semantic and hopefully name that suggests the purpose reasonably but it seems too much. Or just rename Denomination to DisplayDenomination and don't add a new one.

Display sounds like it is exclusively used by the Display trait, and not (for example) by FromStr. If you just mean that this denomination is "for displaying amounts" then Display seems redundant. What else could a denomination be used for?

With @Kixunil's point that we can add trait impls after 1.0 I'm going to chalk this up to not important enough to think about right now and leave the Denomination enum without an Ord or PartialOrd implementation.

Great point. Yeah, let's not add trait impls that we don't agree on unless there's an obvious or known usecase.

And amusingly (since we are considering removing it) folk can just use ArbitraryOrd while they wait for us to implement and release Ord impls for anything that is missing them.

Should we remove Eq/PartialEq until we decide what to do with bit vs microbitcoin?

And amusingly (since we are considering removing it) folk can just use ArbitraryOrd while they wait for us to implement and release Ord impls for anything that is missing them.

Yes, if we are willing to put ArbitraryOrd into our API and commit to it forever.

Should we remove Eq/PartialEq until we decide what to do with bit vs microbitcoin?

In my view it's obvious that they're not equal in the sense of Eq. They're different enum variants that have different names. I'm not sure when you'd want to compare enum variants for "value equality". They are a property of how an amount is displayed.

And amusingly (since we are considering removing it) folk can just use ArbitraryOrd while they wait for us to implement and release Ord impls for anything that is missing them.

Yes, if we are willing to put ArbitraryOrd into our API and commit to it forever.

Oh I mispoke. I was thinking we didn't need the ArbitraryOrd impl here for users to use ordered::Ordered<T>. I was confused (T has to implement ArbitraryOrd)

In my view it's obvious that they're not equal in the sense of Eq.

Yes, the trouble is someone might forget that some variants have duplicated magnitude and they'd assume inequality implies different magnitude. That being said, I'm not sure where people would deal with that if they can just use our parsing/displaying code.

Yeah, behind all this discussion is an open question about what anybody is actually doing directly manipulating the Denomination type.