Introduce boolean-like value types (fixes #13) - wip.

[azasypkin]

PR to handle #13

[azasypkin]

Hey @Yoric I need your help/advise here :)

I'm trying to better understand PartialOrd for different types, and I was thinking that boolean-like enums should implement the same traits as real bool - Eq in addition to PartialEq and PartialOrd, also I found that having as_bool method can be handy (maybe there should be BoolValue {fn as_bool} trait), but still not everything clear in my head - so please let me know if I over-complicating things or introducing bloat.

Thanks!

[Yoric]

I agree that they should implement Eq and PartialEq. Since Value implements PartialOrd, we should also implement it for these new values. And since they are booleans, we might as well go for a full Ord, indeed.

I'm not sure that as_bool is very useful. I think that we prefer pattern-matching against On/Off (which offers no ambiguity) to checking whether as_bool() is true/false (which does). So I wouldn't implement it for the time being.

[azasypkin]

So, no as_bool, but we still need PartialOrd and Ord right? :)

[Yoric]

Yep :)

[azasypkin]

Aaand few more best practices questions for you (sorry for too many, just want to have common ground) :)

• If I delegate PartialOrd and Ord implementation to #derive it won't behave like boolean (true > false, but OnOff::On < OnOff::Off), I suspect we want to have boolean-like behavior and should implement Ord and PartialOrd manually?
• Is it ok to implement PartialOrd via Ord like:

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

Looking at the Ord and PartiallyOrd hierarchy it seems that more naturally would be to implement it the other way around, Ord via PartialOrd, but we'll have addition unwrap call in self.partal_cmp(other).unwrap().

• Is it Ok to use catch-all clause in two-states enum like this, or we should guard ourselves in case one more state will be added?

match (self, other) {
    (&OpenClosed::Open, &OpenClosed::Closed) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open) => Less,
    _ => Equal,
}

vs

match (self, other) {
    (&OpenClosed::Open, &OpenClosed::Closed) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open) => Less,
    (&OpenClosed::Open, &OpenClosed::Open) => Equal,
    (&OpenClosed::Closed, &OpenClosed::Closed) => Equal,
}

or even something crazy :)

match (self, other, self == other) {
    (_, _, true) => Equal,
    (&OpenClosed::Open, &OpenClosed::Closed, _) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open, _) => Less,
}

[Yoric]

Aaand few more best practices questions for you (sorry for too many, just want to have common ground) :)

On the contrary, thanks for trying to setup a common ground!

• If I delegate PartialOrd and Ord implementation to #derive it won't behave like boolean (true > false, but OnOff::On < OnOff::Off), I suspect we want to have boolean-like behavior and should implement Ord and PartialOrd manually?

I suppose that this would be the least surprising.

• Is it ok to implement PartialOrd via Ord like:

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

That's how I would do it.

Looking at the Ord and PartiallyOrd hierarchy it seems that more naturally would be to implement it the other way around, Ord via PartialOrd, but we'll have addition unwrap call in self.partal_cmp(other).unwrap().

I haven't thought of this in terms of hierarchy. But it seems to me that PartialOrd can be derived from Ord naturally, so I'd go for it.

• Is it Ok to use catch-all clause in two-states enum like this, or we should guard ourselves in case one more state will be added?

match (self, other) {
    (&OpenClosed::Open, &OpenClosed::Closed) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open) => Less,
    _ => Equal,
}

vs

match (self, other) {
    (&OpenClosed::Open, &OpenClosed::Closed) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open) => Less,
    (&OpenClosed::Open, &OpenClosed::Open) => Equal,
    (&OpenClosed::Closed, &OpenClosed::Closed) => Equal,
}

or even something crazy :)

match (self, other, self == other) {
    (_, _, true) => Equal,
    (&OpenClosed::Open, &OpenClosed::Closed, _) => Greater,
    (&OpenClosed::Closed, &OpenClosed::Open, _) => Less,
}

Ok, ok, I get it, it would be simpler to introduce as_bool :) Just don't make that method public.

[azasypkin]

Thanks for replies! Handling :)

[azasypkin]

I see two different approaches in the code match *self { ENTITY => .... } and match self { &ENTITY => ....}. So I stick to the former because of [1]. There is no any explanation though :/

[1] https://github.com/rust-lang/rust/blob/master/src/doc/style/features/match.md

[Yoric]

Yes, I generally do the former. I seem to remember stumbling upon a case in which it didn't work, so falling back to the latter. If you wish to fix it, you're more than welcome :)

[azasypkin]

Okay, I think it should be ready for review, so r? @Yoric :)

[Yoric]

Looks good to me, thanks for the patch.
Gasp, should have changed the commit message before merging :/

[azasypkin]

Looks good to me, thanks for the patch.

Thanks! I've learned some good stuff here :)

Gasp, should have changed the commit message before merging :/

Grr, my fault - will take care in the next PRs.