-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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 diagnostics to modbus (Satisfy quality demand) #117591
Add diagnostics to modbus (Satisfy quality demand) #117591
Conversation
Again, modbus doesn't use config flow, thus doesn't have a config entry, so this change doesn't make sense. Modbus is a special case because it can be one platinum without having config flow as it is a protocol integration. Thus the requirement don't fully apply, so we either need to make an exception for modbus or rephrase it (but how much effort do you want to put into wording since only mqtt and modbus fall under this rule) |
Maybe the solution is to simply add a comment to |
And again, integrations without config entries are not excluded. It is not only modbus, but configurations that do not use config_flow in general. Apart from that it can actually be used in modbus if the end-user configures the parameter unique id. |
Integrations without config flow are excluded, because they can't get past silver. So they never have to encounter this requirement in the first place. |
in general that is true, but we have at least one exception, and changing the doc or manifest.py could solve that. or accept the PR even though it's a bit silly. |
It's not possible to accept the PR as it is... it is failing |
Right now modbus will never have a green CI and thus for all practical purposes is dead. An alternative (although not very nice) is to remove the modbus integration, with the reason that new PRs are not possible and assume it reappears as a custom component. Degrading to silver, would have the negative effect, that it would block a custom component since it would fix the pymodbus version. Please advice which road I as the active maintainer should choose. |
Thus my suggestion: add a comment in |
That is a good solution, that file as far as I can see, belong to the core team (who added modbus in the first place). So I assume the correct procedure is to close this PR and hope manifest.py gets corrected. The reason this PR fails is because I did not remove modbus from the no_diagnostic list, which of course should be done with this PR (will do it later today). |
manifest is updated, so hopefully CI goes green. WIth this PR, the “demand” in #117576 is full filled. Since modbus was added to manifest.py, and the text do not exclude modbus, I cannot “just” comment modbus out with a comment. Please remember this PR actually works, if the user uses the parameter “unique id”. |
Please check Discord |
Breaking change
Proposed change
This add diagnostic information to modbus as requested in #117576.
As per home-assistant/developers.home-assistant#1512, gold and platinum integrations should implement diagnostic ! (modbus is platinum).
The modbus protocol do not offer diagnostic information, so this PR simply returns the current data (something which
is already available in the UI for each entity).
The diagnostic information does not replace the need for a log with debug information, when creating an issue !!
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: