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

Add adapter to repo #12

Open
9 of 12 tasks
jjqoie opened this issue Jan 1, 2024 · 2 comments
Open
9 of 12 tasks

Add adapter to repo #12

jjqoie opened this issue Jan 1, 2024 · 2 comments
Assignees

Comments

@jjqoie
Copy link
Owner

jjqoie commented Jan 1, 2024

          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 (

    onMessage(obj) {
    ). 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.

    this.adapter.setObjectNotExists(this.name + ".ShareFile.NextcloudPath", {

  • 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)

    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

    this.adapter.log.info("CreateUploadPathIfnotExist " + this.config.upload_path);

    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!

Originally posted by @mcm1957 in ioBroker/ioBroker.repositories#2150 (comment)

@DaGo1120
Copy link

DaGo1120 commented Jan 5, 2025

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.

@mcm1957
Copy link
Contributor

mcm1957 commented Jan 5, 2025

Installing an adapter directly form Github is strongly discouraged unless instructed by developer i.e. to aid during debugging.

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

When branches are created from issues, their pull requests are automatically linked.

3 participants