intel / iotivity-node

Node.js bindings for IoTivity
https://www.iotivity.org/
42 stars 44 forks source link

server and client demo #147

Closed alshafi closed 7 years ago

alshafi commented 7 years ago

These examples were used to demo IoTivity-node on the Raspberry-pi Zero W board with the Envirophat sensor board which included temperature sensor, accelerometer sensor, light-color sensor, and controlling the led on and off.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling 58c8da3309a8efd0881c4f6b20808044b0791d8b on alshafi:master into ce496720c1d954335e07072d3a4c3f3862f71708 on otcshare:master.

alshafi commented 7 years ago

I just added 3 files. I am not sure how that would fail CI. Does CI failure block this pull request from being merged into the main repo? Thanks, -Rami

gabrielschulhof commented 7 years ago

The files you added do not conform to the style: https://travis-ci.org/otcshare/iotivity-node/jobs/268081057#L459

Most of those errors stem from the fact that the files have Windows-style line endings (\r\n) instead of Unix-style (\n).

gabrielschulhof commented 7 years ago

@alshafi if you're working on Windows, you should run

git config core.autocrlf input

in the iotivity-node repo. That will tell git to check out files as is, but to change the line endings to Unix when committing.

After doing that, you can check out this commit and re-commit with

git commit --amend --no-edit

This should convert the line endings to Unix, making a lot of those errors go away. Some will still be left, because you variously use single and double quotes, and you variously leave and do not leave space on the inside of brackets.

alshafi commented 7 years ago

@gabrielschulhof Thanks! I knew my commit failed CI but I could not see those errors yesterday. Now I do. Travis seems to be in progress now.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling 75521908d66b6087663cfc72335379013bdbe9a4 on alshafi:master into ce496720c1d954335e07072d3a4c3f3862f71708 on otcshare:master.

gabrielschulhof commented 7 years ago

@alshafi you can run npm -g install grunt-cli followed by grunt test locally to see if the tests pass.

alshafi commented 7 years ago

@gabrielschulhof it did not work for me. The installation seemed good but I keep getting a fatal error for not finding local grunt when I try grunt test. I googled the solution and trying grunt init and that resulted in the same error of not finding the local grunt. it would be great if i could get those errors locally. Also, can you please confirm that my amended commit made it to this pull request? Thanks, -Rami

gabrielschulhof commented 7 years ago

@alshafi you need to also npm install in the root directory of the repo to be able to run grunt.

The commit made it because it appears after @nagineni's comment.

hansmbakker commented 7 years ago

@gabrielschulhof

hansmbakker commented 7 years ago

@gabrielschulhof that issue with crlf / lf can be fixed for all contributors in one place by configuring it in a file called .gitattributes

gabrielschulhof commented 7 years ago

@wind-rider Thanks for the tip!

gabrielschulhof commented 7 years ago

@wind-rider I actually added that in https://github.com/otcshare/iotivity-node/issues/131, so I don't understand how CRLFs can sneak in.

gabrielschulhof commented 7 years ago

@alshafi looking more closely at your PR, it looks like this is more than just an example. You could publish this as a npm package in its own right. For example, you could call it envirophat-ocf, similar to envirophat-mqtt.

gabrielschulhof commented 7 years ago

@alshafi such a package may actually be a more appropriate place for this code.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling 20d1e1b10dbe06451fce2f58c5589e490ef450dc on alshafi:master into ce496720c1d954335e07072d3a4c3f3862f71708 on otcshare:master.

alshafi commented 7 years ago

@gabrielschulhof running npm install in the root directory fixed my grunt issues and was able to run it locally. fixed all of the CI reported issues and pushed the latest changes. Hopefully, it passes now. Thank you! I am not familiar with making npm packages nor do I have the time to learn it. I just need to publish those examples in this repository as many OCF developers are asking me for them and I have been sending them via e-mail. It would be much more convenient if those examples were included into this repo and I will be maintaining them as needed. Thanks, -Rami

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling 8beef2645c506a0f072dca356a7c80687dddf20b on alshafi:master into ce496720c1d954335e07072d3a4c3f3862f71708 on otcshare:master.

alshafi commented 7 years ago

Now I do not understand why CI is failing...

poussa commented 7 years ago

@gabrielschulhof @alshafi Lets host the code here now and move to npm, if needed, at some later point in time.

gabrielschulhof commented 7 years ago

@alshafi the Windows CI needs some love. I have a fix ready and I need to merge it into master, after which it'll pass.

gabrielschulhof commented 7 years ago

@alshafi LGTM in general, however, the code could use some more comments, especially since these are examples. In particular, it needs an overview of what "envirophat" is and what system prerequisites it has before the example can be run.

alshafi commented 7 years ago

@gabrielschulhof I am not sure what LGTM stand for but I plan to document everything. Once the code gets into the repo and a few OCF developers can use my examples successfully (with my assistance if needed), then I will get help from a professional technical writer to improve the documentations for this example. In the mean time, do you want me to past the documentaions in this PR request or a readme file or in the github wiki?

dbkinder commented 7 years ago

LGTM=Looks Good To Me

alshafi commented 7 years ago

Parts list:

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling 821199a506944adfec15c5777addc16890dbfeed on alshafi:master into ce496720c1d954335e07072d3a4c3f3862f71708 on otcshare:master.

gabrielschulhof commented 7 years ago

@alshafi actually, it might be best to move your example to a subdirectory like js/envirophat. The reference require("iotivity-node/lowlevel") will continue to work from the code, because IIRC Node examines all node_modules subdirectories, even those of parent directories.

I can do this when I merge your PR.

alshafi commented 7 years ago

done, just created the envirophat folder inside js and included the documentation in a README.md file in t he envirophat folder

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 87.015% when pulling add222844bdaa3541817633aafb47bf8eb086900 on alshafi:master into 2b0aa5ea2e9ea9ea80804328c7c826afaed7dfcb on otcshare:master.

alshafi commented 7 years ago

Awesome! looks like everything is green and passing. What is next to merge this PR ?