Decentralizing: Removing big centralized Value #520

Yoric · 2016-06-08T21:34:13Z

This PR removes the big centralized enum Value, effectively ending all centralization in Taxonomy. The first two beneficiaries are Thinkerbell (which now defines its own value type RuleSource) and WebPush (which now defines its own value type WebPushNotify).

r? @fabricedesre for Foxbox.
r? @mcav for Thinkerbell.
r? @julienw for ZWave.
r? @cr for Philps Hue.
r? @dhylands for IP Camera.
r? @aosmond for WebPush.
r? @fabricedesre for TTS.
r? @npark-mozilla for integration tests.
f? @azasypkin for synchronization with front-end.

I still have a few PRs planned after this lands, but at this stage, Taxonomy will be Decentralized \o/

aosmond · 2016-06-08T21:38:11Z

r=me for WebPush.

mcav · 2016-06-08T22:04:19Z

thinkerbell lgtm

fabricedesre · 2016-06-08T22:24:59Z

components/taxonomy/src/lib.rs

@@ -4,6 +4,8 @@
 #![plugin(clippy)]
 // To prevent clippy being noisy with derive(...)
 #![allow(used_underscore_binding)]
+#![allow(let_unit_value)] // For some reason, clippy decides to display this warning, without 


nit: finish that sentence and remove extra blank line.

fabricedesre · 2016-06-08T22:44:04Z

r=me thanks!

dhylands · 2016-06-09T04:21:59Z

src/adapters/ip_camera/upnp_listener.rs

-            udn = String::from(&udn[5..]);
-        }
+            .trim_left_matches("uuid:")
+            .to_owned();


nit: Thanks for the rewrite. I'd probably put the comment in front of the let. I think the comment in the middle of a statement looks funny.

Note that I rewrote that only because Clippy kept pestering me about the style of this statement.

I filed a bug with clippy, and it's been fixed in the latest version (not sure when it will be released).
See: https://github.com/Manishearth/rust-clippy/issues/975

Looks like it should be fixed in release 0.0.72

dhylands · 2016-06-09T04:22:41Z

r=me for camera stuff.

Yoric · 2016-06-09T06:42:43Z

Adding:

r? @fabricedesre for TTS (which I had forgotten in the previous PR);
Cc @azasypkin because of upcoming breakages.

Yoric · 2016-06-09T07:02:52Z

Adding r? @npark-mozilla for integration tests.

Also, @azasypkin , you can now look at the integration tests to see the API changes.

coveralls · 2016-06-09T07:45:10Z

Coverage decreased (-0.4%) to 32.594% when pulling 4ced0a6 on Yoric:refactor-7 into 711ea22 on fxbox:master.

cr · 2016-06-09T07:56:57Z

r+ for the Hue changes and the changes to the Color type.

julienw · 2016-06-09T09:59:10Z

components/openzwave-adapter/src/lib.rs

    fn should_send(&self, value: &Value, _: EventType) -> bool {
-        self.contains(value)
+        self == value // FIXME: This won't scale up to interesting ranges.


what's the path forward with this FIXME? Maybe you could file an issue and put the issue # here and above ?

Well, this FIXME is actually a reminder that should_send only works for simple cases. For more complicated cases, well, a different algorithm will be needed. As I have no clue about which cases we are going to handle in the future, I don't really see how I can be much more specific.

azasypkin · 2016-06-09T11:31:55Z

f+ from the client side (app related PR is fxbox/app#172)!

Just a few notes (maybe they are the same on master though, didn't check):

When incorrect rule is saved (e.g. without execute part), there is very vague error is returned {"thinkerbell-add-rule":{"Error":"Internal Error"}};
It was there before, but since you're updating related pieces: it's a bit hard to distinguish successful response with value and failed one (especially for batch requests where response code should be 200 in any case):

{"OpenZWave-d32394d4-01000000024c0000":"Closed"};
{"OpenZWave-d32394d4-01000000024c0000":{"Error":{"OperationNotSupported": ...}}

Currently we rely on Error property of returned object, but that's dumb :) Theoretically valid value can be object with Error property. Something like this {"OpenZWave-d32394d4-01000000024c0000":{ value: "Closed", error: {"OperationNotSupported": ...}} would probably be less confusing.

julienw · 2016-06-09T12:14:15Z

components/taxonomy/src/parse.rs

@@ -168,7 +178,15 @@ impl ParseError {
 }

 #[derive(Debug)]
-pub struct JSONError(error::Error);
+pub struct JSONError(pub error::Error);


just wondering why you need the "pub" for the argument ?

Er... I don't remember, but I remember that I do :)

Yoric · 2016-06-09T12:20:08Z

When incorrect rule is saved (e.g. without execute part), there is very vague error is returned {"thinkerbell-add-rule":{"Error":"Internal Error"}};

Almost sure that's not new. Looks like something that should be solved as part of #519.

It was there before, but since you're updating related pieces: it's a bit hard to distinguish successful response with value and failed one (especially for batch requests where response code should be 200 in any case):
[...]
Currently we rely on Error property of returned object, but that's dumb :) Theoretically valid value can be object with Error property. Something like this {"OpenZWave-d32394d4-01000000024c0000":{ value: "Closed", error: {"OperationNotSupported": ...}} would probably be less confusing.

Good idea, shouldn't be too hard. Can you file an issue?

julienw · 2016-06-09T12:22:57Z

components/taxonomy/src/values.rs

+        }
+    }
+}
+mopafy!(Data);


so that I understand properly, this is how we'll let adapters implement their own data types ?
Mopa is like an "open" enum ?

Basically, yes. Mopa is a more flexible + type-safe Any that can be used much more comfortably to implement an open enum.

julienw · 2016-06-09T12:23:42Z

components/taxonomy/src/values.rs

+    /// A human-readable description of the _type_ of the value.
+    ///
+    /// Used mainly in `TypeError` error messages.
+    fn description() -> String where Self: Sized;


wondering if this could be a &'static str like for Errors ?

actually I don't really care so just forget this comment :)

It makes things a bit more complicated for Range<T>, unfortunately.

julienw · 2016-06-09T13:46:18Z

r=me, I left some nits and questions but not really important.

We noticed that a DoorLock now uses Open/Closed instead of Locked/Unlocked, and I found that's because DOOR_IS_LOCKED uses Open/Closed. Is it expected ?

azasypkin · 2016-06-09T14:01:20Z

Good idea, shouldn't be too hard. Can you file an issue?

Done #522!

nojunpark · 2016-06-09T14:15:57Z

r+ for me. glad to see the syntax got simpler. It would be awesome if the wiki (https://wiki.mozilla.org/Connected_Devices/Projects/Project_Link/Taxonomy) is updated as well :)

Yoric · 2016-06-09T15:10:47Z

We noticed that a DoorLock now uses Open/Closed instead of Locked/Unlocked, and I found that's because DOOR_IS_LOCKED uses Open/Closed. Is it expected ?

I'll fix this.

fabricedesre · 2016-06-09T16:15:34Z

r=me for tts

coveralls · 2016-06-10T08:49:38Z

Coverage decreased (-0.3%) to 32.614% when pulling cc7dd5c on Yoric:refactor-7 into 711ea22 on fxbox:master.

fabricedesre reviewed Jun 8, 2016
View reviewed changes

dhylands reviewed Jun 9, 2016
View reviewed changes

David Rajchenbach-Teller added 9 commits June 9, 2016 08:31

Taxonomy: Getting rid of centralized Value

31224a3

Thinkerbell: Getting rid of centralized Value

90366d4

ZWave: Getting rid of centralized Value

bf37da3

Clock: Getting rid of centralized Value

15b89dc

Console: Getting rid of centralized Value

44cf2da

Camera: Getting rid of centralized Value

5cc0d0f

Hue: Getting rid of centralized Value

d508067

WebPush: Getting rid of centralized Value

6672c77

Foxbox: Getting rid of centralized Value

a1e5a04

Yoric force-pushed the refactor-7 branch from 03b9dda to a1e5a04 Compare June 9, 2016 06:32

TTS: Getting rid of centralized Value

3e06a43

Integration tests: Getting rid of centralized Value

4ced0a6

julienw reviewed Jun 9, 2016
View reviewed changes

azasypkin mentioned this pull request Jun 9, 2016

API error response should be clearly distinguishable from the successful one #522

Open

Taxonomy: Using IsLocked for DOOR_IS_LOCKED

cc7dd5c

Yoric force-pushed the refactor-7 branch from e4ad4c3 to cc7dd5c Compare June 10, 2016 08:05

Yoric merged commit ba83e11 into fxbox:master Jun 10, 2016

Yoric mentioned this pull request Jun 16, 2016

Thinkerbell Adapter now expects rules as an object instead of a serialized string. #529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decentralizing: Removing big centralized Value #520

Decentralizing: Removing big centralized Value #520

Yoric commented Jun 8, 2016 •

edited by fabricedesre

Loading

aosmond commented Jun 8, 2016

mcav commented Jun 8, 2016

fabricedesre Jun 8, 2016

fabricedesre commented Jun 8, 2016

dhylands Jun 9, 2016

Yoric Jun 9, 2016

dhylands Jun 9, 2016

dhylands commented Jun 9, 2016

Yoric commented Jun 9, 2016

Yoric commented Jun 9, 2016

coveralls commented Jun 9, 2016

cr commented Jun 9, 2016

julienw Jun 9, 2016

Yoric Jun 9, 2016

azasypkin commented Jun 9, 2016

julienw Jun 9, 2016

Yoric Jun 9, 2016

Yoric commented Jun 9, 2016

julienw Jun 9, 2016

Yoric Jun 9, 2016

julienw Jun 9, 2016

julienw Jun 9, 2016

Yoric Jun 9, 2016

julienw commented Jun 9, 2016

azasypkin commented Jun 9, 2016

nojunpark commented Jun 9, 2016

Yoric commented Jun 9, 2016

fabricedesre commented Jun 9, 2016

coveralls commented Jun 10, 2016

Decentralizing: Removing big centralized Value #520

Decentralizing: Removing big centralized Value #520

Conversation

Yoric commented Jun 8, 2016 • edited by fabricedesre Loading

aosmond commented Jun 8, 2016

mcav commented Jun 8, 2016

Choose a reason for hiding this comment

fabricedesre commented Jun 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhylands commented Jun 9, 2016

Yoric commented Jun 9, 2016

Yoric commented Jun 9, 2016

coveralls commented Jun 9, 2016

cr commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Jun 9, 2016

azasypkin commented Jun 9, 2016

nojunpark commented Jun 9, 2016

Yoric commented Jun 9, 2016

fabricedesre commented Jun 9, 2016

coveralls commented Jun 10, 2016

Yoric commented Jun 8, 2016 •

edited by fabricedesre

Loading