Skip to content
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

Some Review Comments ... #1

Closed
Apollon77 opened this issue Dec 24, 2019 · 3 comments
Closed

Some Review Comments ... #1

Apollon77 opened this issue Dec 24, 2019 · 3 comments
Assignees

Comments

@Apollon77
Copy link
Contributor

Hi,

to allow an easier addition to the repository later I did a short rough review and here are some comments.

  • Please check your adapter with https://adapter-check.iobroker.in/ and fix the found stuff
  • testing is missing, please add. A blueprint could be https://github.com/ioBroker/ioBroker.template/tree/master/JavaScript - the test directory, some scripts in package.json and the .github directory these days ...
  • Please use https://translator.iobroker.in/ to auto-translate all nws texts for io-package and also the words.js
  • please make sure to store the password only encrypted! Check Adapters like meross from mee for this (two functions in index_m.html and decrypt method in adapter class)
  • make sure to always clear the timeout in "on-unload" ... currently it could be missed when an exception happens with the request. Also: should not the logic https://github.com/StrathCole/iobroker.plenticore/blob/master/main.js#L176 go into the request callback instead of "In parallel"?
  • Please also check if adapter and adapter.log exists before accessing it in on-unload!
  • If your on-unload needs longer then 500ms think about increasing the shutdown time in io-package (see tuya adapter)
  • If you do not need object changes or messages please also do not implement the method
  • Are the roles with "value.info" all correct? Please check https://www.iobroker.net/#de/documentation/dev/stateroles.md
  • You create a lot of objects and then you do dthe login and poll the state values. Because all the object creation runs asynchronous formally you can not be sure that all objects were already created when you want to set the states ... The effect could be that some values will not be visible directly after the first poll but after the next one ...

Personal note: I do not like to use intervals for external communication becaue the interval fires also when last communication was not finished because of timeouts or such. I more like setting a timeout for the next run once the current one is finished ... But up to you :-)

Have fun and great christmas time

@StrathCole
Copy link
Contributor

Thank you. I will look into all this soon.

@StrathCole
Copy link
Contributor

Hello,

as I am not sure if I understood everything correctly and because I am not yet into all the ioBroker adapter development details, I'll ask some questions back.

* Please check your adapter with https://adapter-check.iobroker.in/ and fix the found stuff  

I cannot fix the "travis" and "npm collaborators" things, yet as I have not dealt with npm before. Neither with travis.

* testing is missing, please add. A blueprint could be https://github.com/ioBroker/ioBroker.template/tree/master/JavaScript - the test directory, some scripts in package.json and the .github directory these days ...  

I'll look into this at a later stage.

* Please use https://translator.iobroker.in/ to auto-translate all nws texts for io-package and also the words.js  

I still have to look into how translation of strings in ioBroker adapters is handled, so I have not yet come to that point. But will.

* please make sure to store the password only encrypted! Check Adapters like meross from mee for this (two functions in index_m.html and decrypt method in adapter class)  

Okay, good point. Will check that.

* make sure to always clear the timeout in "on-unload" ... currently it could be missed when an exception happens with the request. Also: should not the logic https://github.com/StrathCole/iobroker.plenticore/blob/master/main.js#L176 go into the request callback instead of "In parallel"?  

Do you mean the exceptions in the helper functions? Those are only used in the login process, so there is no exception afterwards when the polling interval is live.
The parallel handling in L176 following is on purpose because otherwise it could run into a race condition (making a new poll when logout has just finished).

* Please also check if adapter and adapter.log exists before accessing it in on-unload!  

okay.

* If your on-unload needs longer then 500ms think about increasing the shutdown time in io-package (see tuya adapter)  

okay.

* If you do not need object changes or messages please also do not implement the method  

okay, messages will be handled later on so I prepared that. Will remove object change listener though.

* Are the roles with "value.info" all correct? Please check https://www.iobroker.net/#de/documentation/dev/stateroles.md  

okay, will do that.

* You create a lot of objects and then you do dthe login and poll the state values. Because all the object creation runs asynchronous formally you can not be sure that all objects were already created when you want to set the states ... The effect could be that some values will not be visible directly after the first poll but after the next one ...  

To be honest I have no idea how to handle that in an other way. Saw it in other adapters like this so I thought it was sufficient.

Personal note: I do not like to use intervals for external communication becaue the interval fires also when last communication was not finished because of timeouts or such. I more like setting a timeout for the next run once the current one is finished ... But up to you :-)
I will have some thoughts about that ;-) Thanks for the hint, you are right, I do it in web applications like you mentioned, too.

Have fun and great christmas time
Thanks. You too. Great software, by the way!

@StrathCole StrathCole self-assigned this Dec 24, 2019
@StrathCole
Copy link
Contributor

Think I have solved all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants