You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
First of all - THANK YOU for the time and effort you spend to maintain this adapter.
Sorry for the late reply. I did a complete rereview including special attention to the points appollon77 already noted.
Please see my feedback based on my personal oppinion. Please feel free to contact @Apollon77 if you cannot follow my suggestions or want to discuss some special aspects.
package.json should specifiy required nodejs version
Please consider adding an engines clause to package.json. Especially if you use adapter-core 3.x.x or newer nodegs 16 or newer must be reuested:
"engines": {
"node": ">= 16"
},
please verify that all dependencies are up to date
Some dependencies are outdated. Please update to current releases if no special reason to keep an older version exists.
i.e. adapter-core is outdated for sure.
on message handler seems to be unused
There seems to be no fucntional code within onMessage handler (
). If this is correct, please remove onMessage handler and remove '"messagebox": true,' from io-package.json.
onStateChange handler ignores ack flag
The adapter must honor the ack flag when processing onStateChange events. Whenever the onStateChange handler is called for an own state (a state owned by the processing instance of the adapter) it must ignore this event if the ack flags is set. Only stateChanges with ack==false are allowed to be processed normally. Exceptions might exist - please discuss if you fell a need for that.
When processing foreign states (states not created by this adapter) processing most likely should only occure if the ack flag is true - but there might be exceptions.
creation of folders seems to be missing
Every folder (device, folder, ...) must be created explicitly. You may not create a state name.Sharefile.NetxCloudPath without creating a folder (?) ShareFile first.
In addition the roles MUST match the read/write functionality. As an example a role value.* requires that the state is a readable state. Input states (write only) should have roles like level.*. Please read the description carefully. States with role 'button' should (must) have attribute 'common.read=false' set. A button ( "Taster" in german only triggers when you press but else has no state), so it should also be not readable. This is also like HomeMatic does it. A switch has clear states true or false and so can be read.
Please avoid using general roles (state, value) whnever a dedicated role exists.
Many of your states with role state look like text roles. Please review / adapt to rules linked above.
use not too long names for states
The text used here looks more like a description (which ia available too)
name: "Share an already uploaded and existing file",
avoid console.log
console.log should be avoided as it is only useable with a dubugger. Use adapter.log.debug or adapter.log.silly instead.
Note that adapter.log.* output is visible at debugger console too, so theres seems to be no need for console.log at all.
mionimize info logs to contain only really important info
I do not think thats its very importand for users to know every tme when a working directory is checkd to exist
Please check info logs and use i.e. debug or silly for if applicable.
why do you subscribe LastReceivedMessage?
Subscribing on states only written by your own adapter is normally uneccessary overhead. The handler does not do anything more than logging the change. So I suggest to remove the handling part and the subscription adn to move the logging near setState.
github dependabot support seems to be missing
It looks like you did not yet setup dependabot support; at least .github/dependaboit.yml is missing.
Please consider setting up dependabot support (and to ease life the workflow auto-merge.yml). Dependabot will ensure that your adapter uses the most recent versions of external packages and avoids problem due to outdted base libraries. Dependabit provides PRs at you repository to update the dependencies. Depending on you setting, especially on auto-merge.yml configuration no or only minor revisions will be outmatically merged into you main branch. All major revisions msiut (ans should) be reviewed by you.
nextcloud library and timeouts
Apollon77 already noted:
in fact this nctalk library has one big downside ... it never stops. The timeouts used in there are not tracked and can not be
stopped, soideally that should be enhanced because else this will create big issues in compact mode
NextCloud does not provide some clear stop/shutdown entry point (or you do not use it). So currently the adapter canot stop all (!) running timers or asynchronous operations in progress. This will cause very big problems in compact mode as in compact ode the adapter shares a process with other adapters. So when unloading this adapter the timers could possibly access adress space of other adapters.
If you cannot ensure / use a clean stopp/exit functionaliyt of nctalk I suggest to block compact ode for this adapter by setting compact:false at io-package.json.
to anser your Questsions
Would this be onUnload(callback) ??
Question if onUnload is called do you need an immediate reaction or how fast must the adapter been stopped?
Yes onOnload is called whenever a adapter is stopped regularly. Withon onAOnlad every executiong timer, intervall or other asynchronously running code must be stopped. This response should be as fast as possible, as far as I know the default is 500ms which could be extended in general altghough this should be avoided.
Normally adapter regsiter all timer / intvervals createwd and call clearTimer / Intervall during inOnload to abort running timers.
It's up to you to decide wether to implement a clean unload handling or to leave it "direty" and block compact mode. I would accept both.
Thanks for reading and evaluating this suggestions.
McM1957
Please add a comment when you have reviewed and fixed the suggestionsor at least commented the suggestions and you think the adapter is ready for a re-review!
Hi, it's been some time since the last update. What is the current status, will the adapter find its way into the official repo in the foreseeable future?
When I install the adapter via Github, it restarts regularly until it is automatically deactivated by the IOBroker.
Sorry for the late reply. I did a complete rereview including special attention to the points appollon77 already noted.
Please see my feedback based on my personal oppinion. Please feel free to contact @Apollon77 if you cannot follow my suggestions or want to discuss some special aspects.
package.json should specifiy required nodejs version
Please consider adding an engines clause to package.json. Especially if you use adapter-core 3.x.x or newer nodegs 16 or newer must be reuested:
please verify that all dependencies are up to date
Some dependencies are outdated. Please update to current releases if no special reason to keep an older version exists.
i.e. adapter-core is outdated for sure.
on message handler seems to be unused
There seems to be no fucntional code within onMessage handler (
ioBroker.nctalk/main.js
Line 239 in 3686e3a
onStateChange handler ignores ack flag
The adapter must honor the ack flag when processing onStateChange events. Whenever the onStateChange handler is called for an own state (a state owned by the processing instance of the adapter) it must ignore this event if the ack flags is set. Only stateChanges with ack==false are allowed to be processed normally. Exceptions might exist - please discuss if you fell a need for that.
When processing foreign states (states not created by this adapter) processing most likely should only occure if the ack flag is true - but there might be exceptions.
creation of folders seems to be missing
Every folder (device, folder, ...) must be created explicitly. You may not create a state name.Sharefile.NetxCloudPath without creating a folder (?) ShareFile first.
ioBroker.nctalk/lib/iobtalk.js
Line 40 in 3686e3a
reevaluate state roles
Only the values specified here (https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md) may be used as state roles. Do not invent new names as this might disturb the functionalyity of other adapters or vis.
In addition the roles MUST match the read/write functionality. As an example a role value.* requires that the state is a readable state. Input states (write only) should have roles like level.*. Please read the description carefully. States with role 'button' should (must) have attribute 'common.read=false' set. A button ( "Taster" in german only triggers when you press but else has no state), so it should also be not readable. This is also like HomeMatic does it. A switch has clear states true or false and so can be read.
Please avoid using general roles (state, value) whnever a dedicated role exists.
Many of your states with role state look like text roles. Please review / adapt to rules linked above.
use not too long names for states
The text used here looks more like a description (which ia available too)
ioBroker.nctalk/lib/iobtalk.js
Line 43 in 3686e3a
avoid console.log
console.log should be avoided as it is only useable with a dubugger. Use adapter.log.debug or adapter.log.silly instead.
Note that adapter.log.* output is visible at debugger console too, so theres seems to be no need for console.log at all.
mionimize info logs to contain only really important info
I do not think thats its very importand for users to know every tme when a working directory is checkd to exist
ioBroker.nctalk/lib/webdavsupport.js
Line 27 in 3686e3a
Please check info logs and use i.e. debug or silly for if applicable.
why do you subscribe LastReceivedMessage?
Subscribing on states only written by your own adapter is normally uneccessary overhead. The handler does not do anything more than logging the change. So I suggest to remove the handling part and the subscription adn to move the logging near setState.
github dependabot support seems to be missing
It looks like you did not yet setup dependabot support; at least .github/dependaboit.yml is missing.
Please consider setting up dependabot support (and to ease life the workflow auto-merge.yml). Dependabot will ensure that your adapter uses the most recent versions of external packages and avoids problem due to outdted base libraries. Dependabit provides PRs at you repository to update the dependencies. Depending on you setting, especially on auto-merge.yml configuration no or only minor revisions will be outmatically merged into you main branch. All major revisions msiut (ans should) be reviewed by you.
nextcloud library and timeouts
Apollon77 already noted:
NextCloud does not provide some clear stop/shutdown entry point (or you do not use it). So currently the adapter canot stop all (!) running timers or asynchronous operations in progress. This will cause very big problems in compact mode as in compact ode the adapter shares a process with other adapters. So when unloading this adapter the timers could possibly access adress space of other adapters.
If you cannot ensure / use a clean stopp/exit functionaliyt of nctalk I suggest to block compact ode for this adapter by setting compact:false at io-package.json.
to anser your Questsions
Yes onOnload is called whenever a adapter is stopped regularly. Withon onAOnlad every executiong timer, intervall or other asynchronously running code must be stopped. This response should be as fast as possible, as far as I know the default is 500ms which could be extended in general altghough this should be avoided.
Normally adapter regsiter all timer / intvervals createwd and call clearTimer / Intervall during inOnload to abort running timers.
It's up to you to decide wether to implement a clean unload handling or to leave it "direty" and block compact mode. I would accept both.
Thanks for reading and evaluating this suggestions.
McM1957
Please add a comment when you have reviewed and fixed the suggestionsor at least commented the suggestions and you think the adapter is ready for a re-review!
Originally posted by @mcm1957 in ioBroker/ioBroker.repositories#2150 (comment)
The text was updated successfully, but these errors were encountered: