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 rmb-bhkw to latest #2298

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Conversation

satchafunkilus
Copy link
Contributor

@satchafunkilus satchafunkilus commented May 19, 2023

Please add my adapter ioBroker.rmb-bhkw to latest.

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

THIS IS A REPLACEMENT FOR #1779
PLEASE SEE DISCUSSION AT #1779

@satchafunkilus
Copy link
Contributor Author

Referring to PR #1779

@GermanBluefox GermanBluefox added auto-checked This PR was automatically checked for obvious criterias must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review labels May 19, 2023
@mcm1957 mcm1957 added REVIEW needed (apollon77) A developer from the ioBroker Team will review the adapter, will provide comments or require changes and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review auto-checked This PR was automatically checked for obvious criterias labels May 19, 2023
@ioBroker ioBroker deleted a comment from mcm1957 May 19, 2023
@mcm1957 mcm1957 added the auto-checked This PR was automatically checked for obvious criterias label May 19, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented May 19, 2023

RE-CHECK!

@ioBroker ioBroker deleted a comment from mcm1957 May 19, 2023
@GermanBluefox GermanBluefox added the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label May 19, 2023
@ioBroker ioBroker deleted a comment from mcm1957 May 19, 2023
@mcm1957 mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label May 19, 2023
@ioBroker ioBroker deleted a comment from GermanBluefox May 19, 2023
@ioBroker ioBroker deleted a comment from mcm1957 May 19, 2023
@ioBroker ioBroker deleted a comment from mcm1957 May 19, 2023
@mcm1957 mcm1957 added the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Jul 12, 2023
@github-actions github-actions bot deleted a comment from GermanBluefox Jul 17, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Jul 17, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 17, 2023

I've done some reReview resulting in some additional remarks. Please feel free to read them or to wait for official review by Apollon77


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.

  • logging should be english or multilanguage

    Please consider changing the logging text to english wording. As ioBroker is an international software all logging should be either english or in selectable language (which ich noticeable effort and unusual)

  • objects should only be writeable if you expect others to write to those states and handle it.
    As far as I see all states are only written by the adapter, so write should be false for them.

  • invalid characters should be filtered from object ids

  • object ids must (should) not contaizin spaces

    I.e. the following code will result in an state with id 'Ladezustand Warmwasserspeicher'

				results.push({
					name: 'Ladestand Warmwasserspeicher',

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

In addition please ensure that no spaces are within ids. As this might work, some adapter s and especially vis cannot handle ids contain ing spaces. So its highly recommended to omit spaces (and otehr whitespace characters) from ids.

Further please use english text for ids and eitehr use multiplanguage naming (sopported by using an object) or use english names.

  • reevaluate info logging

    Think about changing some info loggings to debug, as I think thats not necessary to log i.e. 'Aktualisiere States in ioBroker.' for every update. This will generate not needed log volume.

  • scheduled adapters should have random delays

    An unmodified schedule adapter that queries data from external systems will produce a lot of issues because usually the default schedule stays unchanged for all users and so all installations out there will then contact the server at the exact same time create a "potentially huge" request spike. The best practice for now is at least to add a random delay to spread the calls within that minute - or even move the "minutes" on first start and so persist that (e.g. see https://github.com/ioBroker/ioBroker.yr/blob/master/main.js#L84-L90) ... if the adapter gets successfull then we have a big issue and the webpage owner contacting us ... we seiould prevent that from the beginning.

  • 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],

  • do not access cloud services at fixed timestamps

    Avoid accessing cloud services at fixed timestamps (i.e. at every full hour, at 5 minutes after midnight , ...) This could lead to high load peaks at the cloud service as all ioBorker installations will use the service at almost the same time. Use random timestamps where possible.

Thanks for reading and evalutaing this suggestions.
McM1957

@satchafunkilus
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 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 Jul 17, 2023
@mcm1957 mcm1957 removed the REVIEW needed (apollon77) A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Aug 7, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Sep 7, 2023

How shall we continue?
Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

@mcm1957 mcm1957 added the question Something needs to be done or answered by the adapter developer label Sep 7, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Nov 26, 2023

@satchafunkilus

Any news?
Do you need help fixing the issues?

Please let us know if you still are woirking / maintain this adapter - a simple I'll continue as soon as I have time would be ok.

If there is no reaction until 15.12.2023 this PR will be closed.

@Apollon77 FYI

@mcm1957 mcm1957 added stale PR seems has no activity, will be closed after some time and removed question Something needs to be done or answered by the adapter developer labels Nov 26, 2023
@satchafunkilus
Copy link
Contributor Author

I'll take a look in the upcoming days

@github-actions github-actions bot added the *📬 a new comment has been added label Nov 29, 2023
@mcm1957 mcm1957 removed stale PR seems has no activity, will be closed after some time *📬 a new comment has been added labels Nov 29, 2023
@satchafunkilus
Copy link
Contributor Author

  • logging should be english or multilanguage
    Please consider changing the logging text to english wording. As ioBroker is an international software all logging should be either english or in selectable language (which ich noticeable effort and unusual)

Changed logging to english only

  • objects should only be writeable if you expect others to write to those states and handle it.
    As far as I see all states are only written by the adapter, so write should be false for them.

Set objects to readonly

  • invalid characters should be filtered from object ids

Done

  • object ids must (should) not contaizin spaces

Done

Further please use english text for ids and eitehr use multiplanguage naming (sopported by using an object) or use english names.

Switched to English

  • reevaluate info logging
    Think about changing some info loggings to debug, as I think thats not necessary to log i.e. 'Aktualisiere States in ioBroker.' for every update. This will generate not needed log volume.

Done

  • scheduled adapters should have random delays

This was already the case - a random delay between 0 and 60 seconds is added to the pull interval

  • adapt testing to supported node releases

Added tests for node 20

  • do not access cloud services at fixed timestamps

This is not the case - using intervals with random delay rather than fixed timestamps

@github-actions github-actions bot added the *📬 a new comment has been added label Nov 30, 2023
@mcm1957 mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Nov 30, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Dec 9, 2023
Copy link

github-actions bot commented Dec 9, 2023

Automated adapter checker

ioBroker.rmb-bhkw

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W400] Cannot find "rmb-bhkw" in latest repository

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

@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 9, 2023

Some more (non blocking) remarks:

  • consider reducing info logs to absolut minimum. In most cases users will consider logs like 'successful pulled data', 'starting ...' as unwanted 'spam'. But you may wait for feedback too if preferred.

  • require node 16
    As this adapter uses adapter-core 3.x.x. in the meantime node 16 is required. I've created a PR for you. Please check and merge. node 16 required due to adapter-core 3.x.x satchafunkilus/ioBroker.rmb-bhkw#134

@mcm1957 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 Dec 9, 2023
@mcm1957 mcm1957 merged commit 2013961 into ioBroker:master Dec 9, 2023
4 checks passed
@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 9, 2023

@satchafunkilus

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 Adapter " 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-checked This PR was automatically checked for obvious criterias lgtm Looks Good To Me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants