✨ add device Philips Hue LWA001 #853
Conversation
|
That if the lint phases was done in the CI ? |
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
=======================================
Coverage 93.49% 93.49%
=======================================
Files 428 428
Lines 5578 5579 +1
=======================================
+ Hits 5215 5216 +1
Misses 363 363
Continue to review full report at Codecov.
|
|
Thanks for your first PR ! 🚀 About the linting I recommend using an IDE (like VS Code) with the Prettier + Eslint extension installed + “Format on save”/“Format on paste” parameter activated. With that, your code will be formatted and linted each time you save a file. Example:
About pre-commit hooks I’m not really in favor of pre-commit hooks in general (even pre-push hooks). With pre-commit hooks, committing is slower and do not work consistently (when prettier returns an error for example). Git should be as fast as possible, so that developers can commit as much as they wants. We want to incentivize a rich git history and frequents commits so developers can go back to their previous code as much as possible, and avoid data loss in case of broken/stolen/lost laptop. It can sound paranoid for a one file PR like this one, but many big PRs on Gladys can be worked on for months, so we want to incentivize developers to commit as frequently as possible, even if prettier/eslint is not passing. About linting on CI About the linting on CI, I’m not in favor of it either. I think linting is for the developer, it helps the developer see what’s wrong in his code visually (you should have linting displayed live in your IDE), so he can improve and understand his mistakes. As this article says it nicely “Ironically, The discipline of maintaining a stable build is gained through the experience of breaking the build.” Running tests About the tests, as you modified the backend only, I recommend that your run only the backend tests suites locally. Go to the Here, your didn’t write the tests for your service so the coverage is going down and coverage check is failing. You need to modify the server/test/services/philips-hue/lights.json file and add the JSON of the light you just added. It’ll make sure that your light is well handled now, and that it keeps working in the future. See this changes in the PR #781 https://github.com/GladysAssistant/Gladys/pull/781/files#diff-ba109fc7d8fc8cc0fdab8d523fa70749 |
|
Ok, I understand the main point. That's ok for me, every project has his own rules, and I'm ok to use those ones. About running tests, I was not able to run them due to errors with the node module version. I can't find any package specifying a version for the project. Did I miss something? Anyway, the contribution should be good by now |
I suppose you mean “Node version”, we use Node LTS (Node 12). Without more logs it’s hard to help. Don’t hesitate to create a specific post on our forum if you want help on this. About your changes, you didn’t put the correct informations in the JSON, see the other rows in the file. |
guillaumeLamanda
force-pushed
the
fix/851
branch
from
a0b6596
to
f59b6f7
Compare
Yes, that's the part I missed. I was using node 14, and the tests don't run with this version.
Indeed, I didn't pay enough attention to the JSON structure. Change done ✅ |
There was a problem hiding this comment.
Just a thought: maybe specifying the node version could help future developers?
Good idea, I added the Node version used in the documentation for now!
It can be specified under the engine key in package.json, or nvm is a common solution to do so.
What do you think?
Good idea for the engine key :)
So I just reviewed your code, and it seems your PR is breaking the LOM002 integration. If you have a close look at the switch case, the LOM002 used to return an ON/OFF plug device, and now it returns a Philips Hue white light.
Can you fix this?
It makes me think that tests are not strict enough because this should have been caught in tests.
Before we improve philips hue tests, can you fix your code + add a comment on the case 'LWA001':l line because when another developer will read this file, he won't understand what is a "LWA001" and can make the same mistake as you :)
By the way, the getPhilipsHueWhiteLight line is now two times in the file and it should be there only once.
You should put your case statement before the first getPhilipsHueWhiteLight()
guillaumeLamanda
force-pushed
the
fix/851
branch
from
5a39051
to
ae675b6
Compare
Awesome!
Indeed. It is corrected by now. 😄
When I looked at the test case, it seems ok IMO. But maybe the controller should be handled by another file dedicated to controllers? It is a bit strange to have controllers in a file called Another thought: I have seen Ikea light in this folder called |
|
Thanks for fixing, it's way better now.
What I meant, is that our tests should verify that the expected light object are the expected one, right now we don't really verify in tests, thus validating your PR despite it being broken.
The The tests code is in files like this one: https://github.com/GladysAssistant/Gladys/blob/master/server/test/services/philips-hue/light/light.getLights.test.js
Yes, IKEA lights are philips hue compatible and handled in the Philips Hue service the same way |
|
@all-contributors please add @guillaumeLamanda for code |
|
I've put up a pull request to add @guillaumeLamanda! 🎉 |
* ✨ add device Philips Hue LWA001 * ✅ add LWA001 test data Co-authored-by: Pierre-Gilles Leymarie <pierregilles.leymarie@gmail.com>
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
I just added the JSON data
npm teston both front/server).npm run eslinton both front/server)npm run prettieron both front/server)front/src/config/demo.json) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
This pull request adds the compatibility for the Philips Hue LWA001 #851