add govee-local to latest

[boergegrunicke]

No description provided.

[mcm1957]

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.

• admin gui / configuration

  Within code you use at least two config paramaters (this.config.searchInterval, this.config.refreshInterval) but you did not define aan admin gui within jsonConfig.json to set them. Although its not forbidden to add "private" data at this.config, I would recommend:

  • to extend jsonConfig.json to add configuration for thoase values (check mcm1957/jsonConfig.sample if you need samples)
  • add all paramaters mit default values to io-package.json.

  If you do not want to provide an configuration possibility, I would sugegst to store the values NOT at this.config. And in addition add some fixed text at jsonConfig.json to indoicate that theres no config - the empty config could cause misunderstandings.

• invalid characters should be filtered from object ids

  It looks like you dtermine the ids of objects / states directly from the external message received.

  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, '_');
}

Although the format of the data might be defined by the manufacturer, keep in mind that due to transmission errors or
later changes of the manufacturer illegal characters coud be introduced. So filtering / converting them will save much troubles and should be implemented. If possible some plausability check would be good too - but that depends on protocal and your decision. (I.e. A responde "404 - no connection" should not cerate a device with this name :-) ). As you use a prefined list at getDataPointDescription it looks like that 'key' is a very limited set.

• possible copy / paste error

  Are you sure that this translation is ok ?

   		case 'ip':
			return 'Specific model of the Lamp';

• clearing intervals

  You create a refreshIntervall for every device:
  https://github.com/boergegrunicke/ioBroker.govee-local/blob/7d9c47887485bacee6ee96b8655ccd40a088d7ac/src/main.ts#L126

  I do not see that this interval is reset / cleared during onUnload. Although this.setInterval ensures some housekeeping, its a good praxis to cancel all intervall times within adapter code on unload.

  In addtion I would ask / suggest to avoid setting a timer for every device using the same tiome interval but to use one time which scans the devices and processes the required tasks. But pleas note that there might by facts I did not see to require single timers

• check and adapt testing

  Standard iobroker testing is required but seems to be missing. Please add and setup ./gizthub/workflows/test-and-release.yml

• adapt testing to supported node releases

  Tests for node 16 is mandatory unless you require node 18 and higher. (OK)
  Tests for node 18 is mandatory as many users already use node 18 and node 16 will be eol and deprected Q3/2023. (OK)
  Consider adding node 20 tests unless you already know incompatibilities.

  So the recommended testmatrix is [16.x, 18.x, 20.x],

• Translations of goovee

  titleLang and descritpions have goovee translated. Please check whether this is ok or goovee should be static as acompany name within all languages

"titleLang": {
			"en": "Govee Local",
			"de": "Govee Lokal",
			"ru": "Гови Местный",
			"pt": "Govee local",
			"nl": "Govee Lokaal",
			"fr": "Gouverner local",
			"it": "Govee Local",
			"es": "Gobierno Local",
			"pl": "Lokalny Govee",
			"uk": "Гові місцевий",
			"zh-cn": "当地政府

Thanks for reading and evalutaing this suggestions.
McM1957

[mcm1957]

might solve ioBroker/AdapterRequests#510

[mcm1957]

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

I would like to give some (additional and 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.

• admin gui / configuration (still open / uncommented)

  Within code you use at least two config paramaters (this.config.searchInterval, this.config.refreshInterval) but you did not define an admin gui within jsonConfig.json to set them. Although its not forbidden to add "private" data at this.config, I would recommend:

  • to extend jsonConfig.json to add configuration for thoase values (check mcm1957/jsonConfig.sample if you need samples)
  • add all parameters with default values to io-package.json.

  If you do not want to provide an configuration possibility, I would sugegst to store the values NOT at this.config. And in addition add some fixed text at jsonConfig.json to indoicate that theres no config - the empty config could cause misunderstandings. As an alternative disabling the comfig panel completly is possible as far as I know. Please ask at telegram for details or mention Apollon77 here to get details if this is desired.

• invalid characters should be filtered from object ids (still open / uncommented)

  It looks like you dtermine the ids of objects / states directly from the external message received.

  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, '_');
}

Although the format of the data might be defined by the manufacturer, keep in mind that due to transmission errors or
later changes of the manufacturer illegal characters coud be introduced. So filtering / converting them will save much troubles and should be implemented. If possible some plausability check would be good too - but that depends on protocal and your decision. (I.e. A responde "404 - no connection" should not cerate a device with this name :-) ). As you use a prefined list at getDataPointDescription it looks like that 'key' is a very limited set.

• Please review logging levels

  Standard logging ioncludes info level. So its in almost all cases not desired to i.e. log every state change as info (https://github.com/boergegrunicke/ioBroker.govee-local/blob/6112b477fef13fe297002af390f4178e01e67314/src/main.ts#L251C1-L251C1). This will most likely result in much to high logging actrivity. Use level debug for logging of internal events and debug tracing instead. (Please check complete code - links is only an example)

• create new release with fixes

  According to commit history you have fixed several issues (Commits on Jul 17, 2023). Those look good. Please create a new release to incorporate them into the npm available to users.

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!

[boergegrunicke]

@mcm1957
Thanks for all the comments. I already started some changes. Due to vacation not all is done so far. As soon as all changes are finished, I will add comments mentioning all single points.

[github-actions]

Automated adapter checker

ioBroker.govee-local

👍 No errors found

• 👀 [W400] Cannot find "govee-local" in latest repository

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

[boergegrunicke]

@mcm1957
Thanks for all the suggestions. I think I changed all of them now.
About having different individual timers for refreshing device states: I wanted to have all intervals consistently for each individual device, this would mainly take effect if the device search interval was higher than the device status refresh interval. I am aware that this is a very theoretical thing with the current implementation, but I would like to keep it like this for now. Maybe there will be a more valid use case. But I keep this in mind, perhaps I will change it in future updates.

[mcm1957]

lgtm now

[mcm1957]

@boergegrunicke

This adapter has been released to latest repository and should be visible within 24h maximum.

Please create a thread at https://forum.iobroker.net/category/91/tester titled like "Test Adapboerter " to collect some user feedback and provide a link to this topic when requesting addition to stable repository later.

Note: If an other testing topic already exists, ist OK to continue using this topic too.