Skip to content

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

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

Merged
merged 1 commit into from
Mar 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use serde::de::{Deserialize, Deserializer, Error};
pub struct ServiceId;

/// Metadata on a service. A service is a device or collection of devices
/// that may offer services. The FoxBox itself a service offering
/// services such as a clock, communication with the user through her
/// that may offer services. The FoxBox itself is a service offering
/// services such as a clock, communicating with the user through her
/// smart devices, etc.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Service {
Expand Down Expand Up @@ -134,13 +134,14 @@ impl ChannelKind {
/// Get the type of values used to communicate with this service.
pub fn get_type(&self) -> Type {
use self::ChannelKind::*;
use values::Type::*;
use values::Type;
match *self {
Ready => Unit,
OnOff | OpenClosed => Bool,
CurrentTime => TimeStamp,
CurrentTimeOfDay | RemainingTime => Duration,
Thermostat | ActualTemperature => Temperature,
Ready => Type::Unit,
OnOff => Type::OnOff,
OpenClosed => Type::OpenClosed,
CurrentTime => Type::TimeStamp,
CurrentTimeOfDay | RemainingTime => Type::Duration,
Thermostat | ActualTemperature => Type::Temperature,
Extension { ref typ, ..} => typ.clone(),
}
}
Expand Down
133 changes: 123 additions & 10 deletions src/values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,16 @@ pub enum Type {
/// has reached 0 or that a device is ready.
Unit,

/// A boolean. Used for instance for on-off switches, presence
/// detectors, etc.
Bool,
///
/// # Boolean values
///

/// A boolean on/off state. Used for various two-states switches.
OnOff,

/// A boolean open/closed state. Used for instance for doors,
/// windows, etc.
OpenClosed,

///
/// # Time
Expand All @@ -56,6 +63,8 @@ pub enum Type {
Color,
Json,
Binary,

ExtBool,
ExtNumeric,
}

Expand All @@ -67,11 +76,68 @@ impl Type {
use self::Type::*;
match *self {
Duration | TimeStamp | Temperature | ExtNumeric | Color => false,
Unit | Bool | String | Json | Binary => true,
Unit | String | Json | Binary | OnOff | OpenClosed | ExtBool => true,
}
}
}

/// An on/off state. Internal representation may be either On or Off.
#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
pub enum OnOff {
On,
Off,
}

impl OnOff {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for replies! Handling :)

fn as_bool(&self) -> bool {
match *self {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

OnOff::On => true,
OnOff::Off => false,
}
}
}

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

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

/// An open/closed state. Internal representation may be either
/// Open or Closed.
#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
pub enum OpenClosed {
Open,
Closed,
}

impl OpenClosed {
fn as_bool(&self) -> bool {
match *self {
OpenClosed::Open => true,
OpenClosed::Closed => false,
}
}
}

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

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

/// A temperature. Internal representation may be either Fahrenheit or
/// Celcius. The FoxBox adapters are expected to perform conversions
/// to the format requested by their devices.
Expand Down Expand Up @@ -123,6 +189,40 @@ impl PartialOrd for Json {
}
}

/// A data structure holding a boolean value of a type that has not
/// been standardized yet.
#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
pub struct ExtBool {
pub value: bool,

/// The vendor. Used for namespacing purposes, to avoid
/// confusing two incompatible extensions with similar
/// names. For instance, "[email protected]".
pub vendor: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note: as part of #11, these strings should become atoms. Otherwise, we're going to lose lots of space.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :)


/// Identification of the adapter introducing this value.
/// Designed to aid with tracing and debugging.
pub adapter: String,

/// A string describing the nature of the value, designed to
/// aid with type-checking.
///
/// Examples: `"PresenceDetected"`.
pub kind: String,
}

impl PartialOrd for ExtBool {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
if self.vendor != other.vendor {
return None;
} else if self.kind != other.kind {
return None;
}

self.value.partial_cmp(&other.value)
}
}

/// A data structure holding a numeric value of a type that has not
/// been standardized yet.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
Expand Down Expand Up @@ -162,7 +262,8 @@ impl PartialOrd for ExtNumeric {
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum Value {
Unit,
Bool(bool),
OnOff(OnOff),
OpenClosed(OpenClosed),
Duration(ValDuration),
TimeStamp(TimeStamp),
Temperature(Temperature),
Expand All @@ -171,18 +272,22 @@ pub enum Value {

// FIXME: Add more as we identify needs

/// A boolean value representing a unit that has not been
/// standardized yet into the API.
ExtBool(ExtBool),

/// A numeric value representing a unit that has not been
/// standardized yet into the API.
ExtNumeric(ExtNumeric),

/// A Json value. We put it behind an `Arc` to make sure that
/// cloning remains unexpensive.
/// cloning remains inexpensive.
Json(Arc<Json>),

/// Binary data.
Binary {
/// The actual data. We put it behind an `Arc` to make sure
/// that cloning remains unexpensive.
/// that cloning remains inexpensive.
data: Arc<Vec<u8>>,
mimetype: String
}
Expand All @@ -192,14 +297,16 @@ impl Value {
pub fn get_type(&self) -> Type {
match *self {
Value::Unit => Type::Unit,
Value::Bool(_) => Type::Bool,
Value::OnOff => Type::OnOff,
Value::OpenClosed => Type::OpenClosed,
Value::String(_) => Type::String,
Value::Duration(_) => Type::Duration,
Value::TimeStamp(_) => Type::TimeStamp,
Value::Temperature(_) => Type::Temperature,
Value::Color(_) => Type::Color,
Value::Json(_) => Type::Json,
Value::Binary{..} => Type::Binary,
Value::ExtBool(_) => Type::ExtBool,
Value::ExtNumeric(_) => Type::ExtNumeric,
}
}
Expand Down Expand Up @@ -230,8 +337,11 @@ impl PartialOrd for Value {
(&Unit, &Unit) => Some(Equal),
(&Unit, _) => None,

(&Bool(a), &Bool(b)) => a.partial_cmp(&b),
(&Bool(_), _) => None,
(&OnOff(ref a), &OnOff(ref b)) => a.partial_cmp(b),
(&OnOff(_), _) => None,

(&OpenClosed(ref a), &OpenClosed(ref b)) => a.partial_cmp(b),
(&OpenClosed(_), _) => None,

(&Duration(ref a), &Duration(ref b)) => a.partial_cmp(b),
(&Duration(_), _) => None,
Expand All @@ -245,6 +355,9 @@ impl PartialOrd for Value {
(&Color(ref a), &Color(ref b)) => a.partial_cmp(b),
(&Color(_), _) => None,

(&ExtBool(ref a), &ExtBool(ref b)) => a.partial_cmp(b),
(&ExtBool(_), _) => None,

(&ExtNumeric(ref a), &ExtNumeric(ref b)) => a.partial_cmp(b),
(&ExtNumeric(_), _) => None,

Expand Down