Large set of changes (see details) #53

tjaffri · 2016-08-17T04:44:08Z

There are a few large fixes in here. See below:

Resolves Node package naming conventions #50 - Node package naming conventions. All the package.json files are now in this format.
Resolves Create org.OpenT2T.Sample.SuperPopular.Thermostat schema in OCF format #44 - Thermostat schema in OCF format after many discussions across @jeanpa, @arjun-msft and @jasongin. Thanks everyone for your help!
Resolves Add basic OCF schemas #46 - Basic OCF schemas. These are checked in under the oic.r.* namespaces, and are referenced from the Thermostat schema.
Partial fixes for Modify the Wink Thermostat Translator to be aligned with OCF format #45 - Thermostat translator in OCF format. The Get and Post are currently stubbed out, since I needed to fix Wink thermostat translator fails with a redirect to the Wink homepage #52 first (added that to this PR below as well). Next, will fill out the Get and Post which should be straightforward with the new wink helper (v2 api).
Resolves Update the directory structure of translators repo to use reverse URI based names for translators #43 - reverse URI based names for translators. Thanks everyone for the feedback in that regard.

Sorry this is such a large merge... but needed multiple rounds of feedback of the holistic design. Will try to make future PRs atomic and easier to review one at a time.

Note: Given how large this commit is and all the interim "work in progress" commits, I suggest whoever does the merge does a "squash commits" so we don't pollute the commit history.

…me limited only to the wink thermostat translator (and thermostat schema). This is work in progress with discussions across various folks, and impacts #43 and #45

…d ocf schema files and updated the README for Wink Thermostat. Have not run test automation yet... that's the next step.

…and adds the main attribute.

…https://www.npmjs.com/search?q=opent2t

Using the schema name as ResURI, and add ID to GET QueryString. This will make it easier to implement RAML in a cloud service that implements accessing this schema.

Also fixed the test scripts and package.json to make the tests work, and use the updating naming conventions in the opent2t library.

Fixed thingTranslator naming conventions to match what opent2t library expects, and updated RAML conventions to map to function names in thingTranslator. Test automation is currently failing, but due to an actual issue with the wink translator! So the test is actually working, but the translator is legitmately failing. That will be addressed separately as part of #52.

As part of this, I have moved the wink helper to the v2 api and signficantly cleaned it up. I have fixed the wink thermostat translator, but now the other wink translators need to be fixed as well. I will follow up with subsequent checkins to resolve as part of #40.

sanjaiganesh · 2016-08-17T17:20:40Z

org.opent2t.sample.thermostat.superpopular/org.opent2t.sample.thermostat.superpopular.xml

+    <property name="targetTemperatureLow" type="d" access="readwrite">
+      <description>Minimum target temperature (in Celsius).</description>
+    </property>
+


change units to fahrenheit

Actually, I stuck with Celsius on purpose.... that's the SI unit. May be a little un-american of me, perhaps :)

For what it's worth, the Wink API (for which we are writing the first translator) also uses Celsius units: http://docs.winkapiv2.apiary.io/#reference/device/thermostats

#ByDesign

I agree with using Celsius units here, as long as the type is decimal not integer so there is no loss of precision when converting on either side.

Note the OCF thermostat schema that we've been discussing actually specifies the units in the property JSON.

(I am an American, and I think non-metric units are silly and old-fashioned.)

so we should fix the schemas to "units": "C" ?

In reply to: 75194786 [](ancestors = 75194786)

There's not much point in making changes to the AllJoyn schemas now, until we decide whether and how we are going to align the AllJoyn schemas with OCF. Maybe we will even auto-convert them from OCF.

In reply to: 75208998 [](ancestors = 75208998,75194786)

@sanjaiganesh I think this is #ByDesign. If you disagree, please file a bug.

sanjaiganesh · 2016-08-17T17:22:53Z

    "optimist": "0.6.1",

Are we not adding 'files' field ? Is it not required ?

Refers to: org.opent2t.sample.windowshade.superpopular/org.opent2t.test.windowshade/js/package.json:10 in 0f69203. [](commit_id = 0f69203, deletion_comment = False)

sanjaiganesh · 2016-08-17T17:23:56Z

tjaffri · 2016-08-17T19:44:49Z

@sanjaiganesh per the discussion in #51 we should put that in the build script rather than include files inside the package.json as a fixed entry. Please file a separate tracking issue if you are concerned.

jasongin · 2016-08-17T20:01:58Z

    "optimist": "0.6.1",

Any use of "files" or "main" in the package.json here is going to be wrong because of how the package.json file is in the wrong directory. Both properties will be automatically generated/overwritten by the build script anyway.

In reply to: 240483336 [](ancestors = 240483336)

Refers to: org.OpenT2T.WindowShade.SuperPopular.Sample/org.OpenT2T.Test.WindowShade/js/package.json:10 in 0f69203. [](commit_id = 0f69203, deletion_comment = False)

jasongin · 2016-08-17T20:05:03Z

org.opent2t.sample.thermostat.superpopular/com.nest.thermostat/js/manifest.xml

@@ -3,9 +3,9 @@
  <!-- associated voice handler -->
  <voice id="" />
  <!-- associated schema -->
-  <schema id="org.OpenT2T.Thermostat.SuperPopular.Sample" />
+  <schema id="org.opent2t.sample.thermostat.superpopular" />
  <!-- associated onboarding module -->


Are we intending to support both a single element at the top level as well as one or more elements under a element? The parsing code in my lib PR currently only supports the latter, but I can change it.

This nest thermostat translator has not been updated as part of this PR (none of the other translators have been updated either). So let's ignore whatever that says (other than renaming/casing changes of course).

The Wink Thermostat is under review here. Issue #36 tracks updating all the other translators (Once we agree on this one I can do a series of PRs modifying all the other translators one by one following the same pattern)

jasongin · 2016-08-17T20:07:49Z

This class follows the AllJoyn schema, which is not in sync with the RAML schema of the same name. Since the schema .js file references the RAML version, this translator is not consistent with that.

Refers to: org.opent2t.sample.thermostat.superpopular/com.nest.thermostat/js/thingTranslator.js:91 in 0f69203. [](commit_id = 0f69203, deletion_comment = False)

jasongin · 2016-08-17T20:18:34Z

org.opent2t.sample.thermostat.superpopular/com.wink.thermostat/js/tests/test.js

+var delay = require('delay')
+
+// This boilerplate code is copied from transpiled typescript AVA test code, which enables usage of 'yield' keyword. 
+var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {


I'm concerned that this, and the use of __awaiter and yield below, makes the test code look far more complicated than it needs to be. We want other developers to be able to copy this test code to use for writing tests for translators they develop. If we're not using TypeScript, then we should just write the test code in normal ES6 syntax with .then() callbacks instead of awaits.

This is .js file. I felt this was looking clean and we really want us to use a test runner like ava, for all its benefits. when we setup lab, we know how many total test cases and how many passed etc.. May be we can simplify this by removing this boilerplate code and fixing it.

In reply to: 75198452 [](ancestors = 75198452)

We can have all those benefits while using normal .then() callbacks instead of this __awaiter which is messy and unfamiliar to people. AVA doesn't require TypeScript or await.

In reply to: 75208579 [](ancestors = 75208579,75198452)

This is a great discussion, but I'm hoping we don't need to block this PR for this. @sanjaiganesh I added issue #55 to track this... perhaps you could take a stab at it? For this PR I think this is #WontFix and we can clean it up later (very soon hopefully, perhaps tomorrow/friday).

tjaffri · 2016-08-17T22:36:39Z

@jasongin as long as you see my note about the nest thermostat translator being out of scope for this PR, I think I have addressed all feedback so far.

Do you guys have more feedback?

jasongin · 2016-08-17T23:21:52Z

sanjaiganesh · 2016-08-18T00:08:32Z

tjaffri added 11 commits August 15, 2016 14:50

WORK IN PROGRESS: More refactoring and dir structure changes, this ti…

542ddd1

…me limited only to the wink thermostat translator (and thermostat schema). This is work in progress with discussions across various folks, and impacts #43 and #45

WORK IN PROGRESS: More renaming and dir structure changes. Also, adde…

3b5e947

…d ocf schema files and updated the README for Wink Thermostat. Have not run test automation yet... that's the next step.

Resolved #50, Node package naming conventions. Updates package names …

752387e

…and adds the main attribute.

Updated onboarding package names to align with ones already published: …

f6a1f72

…https://www.npmjs.com/search?q=opent2t

Updated Thermostat RAML to support service GET

3b7168a

Using the schema name as ResURI, and add ID to GET QueryString. This will make it easier to implement RAML in a cloud service that implements accessing this schema.

Updated README with clearer test guidelines

7680980

Also fixed the test scripts and package.json to make the tests work, and use the updating naming conventions in the opent2t library.

Update org.opent2t.sample.thermostat.superpopular.js

8446832

Removed ID field from RAML

73d0059

Merge branch 'dev' of https://github.com/tjaffri/translators into dev

0a51514

Cleaned up RAML resource URI and js

7fb0fc3

msftclas added the cla-required label Aug 17, 2016

sanjaiganesh added the in progress label Aug 17, 2016

tjaffri assigned jasongin Aug 17, 2016

tjaffri added cla-not-required and removed cla-required labels Aug 17, 2016

tjaffri assigned arjun-msft and sanjaiganesh Aug 17, 2016

This was referenced Aug 17, 2016

Minor changes : adding files/main sections in package.json and removing test config file #47

Closed

Switch nest translator to user firebase submodule. #35

Closed

Subscriptions #33

Closed

tjaffri added 2 commits August 17, 2016 04:28

Update README.md

0f69203

sanjaiganesh reviewed Aug 17, 2016
View reviewed changes

jasongin reviewed Aug 17, 2016
View reviewed changes

This was referenced Aug 17, 2016

Add missing RAML for oic.r.baseresource #54

Closed

Clean up ava test automation to be pure js #55

Closed

sanjaiganesh merged commit 396b694 into openT2T:dev Aug 18, 2016

sanjaiganesh removed the in progress label Aug 18, 2016

tjaffri mentioned this pull request Aug 22, 2016

Wink thermostat translator fails with a redirect to the Wink homepage #52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Large set of changes (see details) #53

Large set of changes (see details) #53

tjaffri commented Aug 17, 2016 •

edited

Loading

sanjaiganesh Aug 17, 2016

tjaffri Aug 17, 2016

jasongin Aug 17, 2016

sanjaiganesh Aug 17, 2016

jasongin Aug 17, 2016

tjaffri Aug 17, 2016

sanjaiganesh commented Aug 17, 2016

sanjaiganesh commented Aug 17, 2016 •

edited

Loading

tjaffri commented Aug 17, 2016

jasongin commented Aug 17, 2016

jasongin Aug 17, 2016

tjaffri Aug 17, 2016

jasongin commented Aug 17, 2016

jasongin Aug 17, 2016

sanjaiganesh Aug 17, 2016

jasongin Aug 17, 2016

tjaffri Aug 17, 2016

tjaffri commented Aug 17, 2016

jasongin commented Aug 17, 2016

sanjaiganesh commented Aug 18, 2016

Large set of changes (see details) #53

Large set of changes (see details) #53

Conversation

tjaffri commented Aug 17, 2016 • edited Loading

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

Choose a reason for hiding this comment

sanjaiganesh commented Aug 17, 2016

sanjaiganesh commented Aug 17, 2016 • edited Loading

tjaffri commented Aug 17, 2016

jasongin commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin commented Aug 17, 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

tjaffri commented Aug 17, 2016

jasongin commented Aug 17, 2016

sanjaiganesh commented Aug 18, 2016

tjaffri commented Aug 17, 2016 •

edited

Loading

sanjaiganesh commented Aug 17, 2016 •

edited

Loading