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

new adapter Pylontech health #2714

Merged
merged 1 commit into from
Oct 6, 2023
Merged

new adapter Pylontech health #2714

merged 1 commit into from
Oct 6, 2023

Conversation

PLCHome
Copy link
Contributor

@PLCHome PLCHome commented Oct 1, 2023

This new adapter is used to determine the health status and functions of a Pylontech array, which can consist of one or up to fifteen batteries.

… of a Pylontech array, which can consist of one or up to fifteen batteries.
@github-actions github-actions bot 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 Oct 1, 2023
@mcm1957 mcm1957 added the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Oct 1, 2023
@PLCHome
Copy link
Contributor Author

PLCHome commented Oct 1, 2023

❗ [E201] Bluefox was not found in the collaborators on NPM!. Please execute in adapter directory: npm owner add bluefox iobroker.pylontech

The invitation only has to be accepted then the adapter is error-free

>npm owner add bluefox iobroker.pylontech
npm ERR! code EOWNERMUTATE
npm ERR! Failed to update package: "409 Conflict - PUT https://registry.npmjs.org/iobroker.pylontech/-rev/1-c1b1915ff34c771ee6977eede4de0220 - User already has a pending invite"

@github-actions github-actions bot deleted a comment from PLCHome Oct 3, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 4, 2023

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.

  • errors reported by adapter checker must be fixed

    Especially error [E201] Bluefox was not found in the collaborators on NPM!. must be fixed. If you just invited Bluefox its ignore to wait for his reaction. Please state that the invitation has been sent in this case. (I know that the invitation is sent - just waiting for acceptance)

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

    This is non blocking - just a suggestion.

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 mcm1957 removed the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Oct 4, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Oct 4, 2023
@PLCHome
Copy link
Contributor Author

PLCHome commented Oct 4, 2023

The invitation has been sent but not yet accepted. See above.
The timeout was changed and the version was published.

Thanks for the first comment.

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 5, 2023

Bluefox is offline this week due to holidays as far as I know. Please wait and ev check next eek if invite jas expired

@github-actions github-actions bot deleted a comment from mcm1957 Oct 6, 2023
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Automated adapter checker

ioBroker.pylontech

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI
  • 👀 [W400] Cannot find "pylontech" in latest repository

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

@mcm1957 mcm1957 added the lgtm Looks Good To Me label Oct 6, 2023
@mcm1957 mcm1957 merged commit b1e0989 into ioBroker:master Oct 6, 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 Oct 6, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 17, 2023

Your adapter has been release 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.

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.

3 participants