Add renacidc to latest #2694

raschy · 2023-09-29T11:24:00Z

Please add my adapter ioBroker.renacidc to latest.

This pull request was created by https://www.iobroker.dev c0726ff.

mcm1957 · 2023-10-04T19:07:23Z

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

I would like to give some (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

Readme.md should contain a link to the manufacturer and/or device

The README.md of any adapter related to device or multiple devices of a manufacturer should contain a link the manufacturers website and/or to a description of the device. This is to enable new users to easily check wether they associate the adapter with the correct device(s). (See https://github.com/ioBroker/ioBroker.repositories#requirements-for-adapter-to-get-added-to-the-latest-repository paragraph 4.)
module apiClient has empty lines

consider removing some empty lines from apiCLient (non blocking)
invalid characters should be filtered from object ids

Some characters are not allowed to be used as part of an object id. If an object id is not hardcoded or even depending on user input you should ensure that such an object id does not contain an invalid character. Iobroker provides the constant adapter.FORBIDDEN_CHARS, i.e. by using the following snippet:

function name2id(pName) {
    return (pName || '').replace(adapter.FORBIDDEN_CHARS, '_');
}

As object ids are built using stationId and key you should filter for invalid charactes transmitted by web service. There's always a possibility that the external service changed the data returned and even such a case should not generate invalid states.

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) whenever a dedicated role exists. It looks like you are always using role = value. Setting correct roles (and units) should be possible using a table for a list known keys. If the webservice adds new keys later, one could add the role/unit Info with a new adapter release later.
consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout

The adapter package provides wrapper routines for native setTimeout, setInterval, clearTimeout and clearInterval. Using those routines ensures that timers are cancelled on on load. Additional checks on thomse limits might be performed, too. So consider replacing native setTimeout/clearTimeout by adapter.setTimeout/adapter.clearTimeout or this.setTimeout/this.clearTimeout. The same refers to setInterval/clearInterval.

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!

mcm1957 · 2023-10-09T19:39:05Z

@raschy
Do you need any help?

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!

raschy · 2023-10-09T20:04:32Z

I have edited the comments in the first adapter review in the points 'Readme.md', the interception of 'Forbidden_Chars' and the specification of the 'stateroles' to the best of my ability. I could not understand the reference to the 'setTimeout'.
While working through the empty lines in 'apiClient.js', I came across the warning in VSC: "Property 'default' does not exist on type 'AxiosStatic'. Did you mean 'defaults'?". (See also in starters-adapter-development'.) I can't find a solution to this.

raschy · 2023-10-11T15:06:20Z

@mcm1957 Hopefully I have corrected all the deficiencies. Please review again.

mcm1957 · 2023-10-17T09:07:48Z

I could not understand the reference to the 'setTimeout'.

js provides a set of functions setTimeout / clearTimeout / setInterval / clearInterval

ioBroker adaptercore provides warppers for thos functions named identically but located at the adapter namespace. So ypu can use them by writing adapter.setTimes (classicmode) or this.setTimeout (class mode). The benefit of using the wrappers is, that the adaptercore has some firwealls built in to handle "forgotten" cleanups and has (or will have) checks for parameter valueas so that i.e. values too big do not result in immidate execution or something like this. Simpl adding a tzhsi. in formt of setInterval and clearInterval should do the magic.

I do not consider this blocking - take it as a suggestion for next revision.

mcm1957 · 2023-10-17T10:01:26Z

BUT I think I detected a new issue.

At https://github.com/raschy/ioBroker.renacidc/blob/bd4327bed1585b12b7186aa9f02f9d1926f0ccef/main.js#L115 you remove invalid characters from the key. But I think this conversion is missing for deleteDeviceState. Please check - I do not consider this as blocking too but only as a normal issue / bug (if ist not ok at all anyway).

github-actions · 2023-10-17T10:23:42Z

Automated adapter checker

ioBroker.renacidc

👍 No errors found

👀 [W400] Cannot find "renacidc" in latest repository

ioBroker.harmony

ioBroker.lgtv

👍 No errors found

👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI
👀 [W145] Each "common.news" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)

ioBroker.mqtt-client

👍 No errors found

ioBroker.pushbullet

👍 No errors found

👀 [W109] "common.desc" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
👀 [W145] Each "common.news" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
👀 [W513] "gulpfile.js" found in repo! Think about migrating to @iobroker/adapter-dev package
👀 [W505] setTimeout found in "main.js", but no clearTimeout detected

ioBroker.pylontech

👍 No errors found

👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI

ioBroker.radar2

ioBroker.vis-justgage

❗ [E104] No "common.titleLang" found in io-package.json
❗ [E162] onlyWWW should have common.mode "none" in io-package.json
👀 [W171] "common.title" is deprecated in io-package.json
👀 [W109] "common.desc" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
👀 [W164] Adapters without config "common.noConfig = true" should also set "common.adminUI.config = none"
👀 [W172] "common.localLink" in io-package.json is deprecated. Please define object "common.localLinks": { "_default": "..." }
👀 [W145] Each "common.news" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
👀 [W145] Each "common.news" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
👀 [W202] Version of package.json (1.0.1) doesn't match latest version on NPM (1.0.2)

Add comment "RE-CHECK!" to start check anew

mcm1957 · 2023-10-17T10:31:22Z

Your adapter has been released to latest repository lately.

Please create a forum topic at https://forum.iobroker.net/category/91/tester named 'Test adapter xxxx' to invite users to test the adapter. Some feedback at forum is desired before adapter is released to stable repository in future.

If some other thread discussing the adapter already exists, its ok too - you do not need to open a second topic in this case.

Added renacidc to sources-dist.json

a6f8e28

github-actions bot added the auto-checked This PR was automatically checked for obvious criterias label Sep 29, 2023

mcm1957 added the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Sep 29, 2023

mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Oct 4, 2023

mcm1957 added the question Something needs to be done or answered by the adapter developer label Oct 9, 2023

mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed question Something needs to be done or answered by the adapter developer labels Oct 10, 2023

mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Oct 17, 2023

Merge branch 'master' into renacidc-to-latest

72accb0

mcm1957 added lgtm Looks Good To Me and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Oct 17, 2023

mcm1957 assigned Apollon77 Oct 17, 2023

github-actions bot added the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Oct 17, 2023

github-actions bot deleted a comment from mcm1957 Oct 17, 2023

mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Oct 17, 2023

mcm1957 merged commit e266e6b into ioBroker:master Oct 17, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add renacidc to latest #2694

Add renacidc to latest #2694

raschy commented Sep 29, 2023

mcm1957 commented Oct 4, 2023 •

edited

Loading

mcm1957 commented Oct 9, 2023

raschy commented Oct 9, 2023

raschy commented Oct 11, 2023

mcm1957 commented Oct 17, 2023

mcm1957 commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

mcm1957 commented Oct 17, 2023

Add renacidc to latest #2694

Add renacidc to latest #2694

Conversation

raschy commented Sep 29, 2023

mcm1957 commented Oct 4, 2023 • edited Loading

mcm1957 commented Oct 9, 2023

raschy commented Oct 9, 2023

raschy commented Oct 11, 2023

mcm1957 commented Oct 17, 2023

mcm1957 commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

Automated adapter checker

mcm1957 commented Oct 17, 2023

mcm1957 commented Oct 4, 2023 •

edited

Loading