Add quality scale for GIOS#155603
Conversation
|
Hey there @bieniu, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
mik-laj
force-pushed
the
gios-quality-scale-2
branch
from
b28204b to
d16d33c
Compare
|
I think we managed to meet all the requirements for platinum. Last changes was covered by: home-assistant/home-assistant.io#42740 |
There was a problem hiding this comment.
Pull request overview
This PR adds Platinum-level quality scale support to the GIOS (Polish Chief Inspectorate Of Environmental Protection) integration. The changes enable the integration to be recognized and validated as meeting the highest quality standards defined by Home Assistant's Integration Quality Scale framework.
- Adds comprehensive quality scale rule tracking via
quality_scale.yaml - Declares Platinum-tier quality scale in the integration manifest
- Removes GIOS from quality scale exemption lists in the validation tooling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
homeassistant/components/gios/quality_scale.yaml |
Adds complete quality scale rule status tracking for Bronze, Silver, Gold, and Platinum tiers with appropriate exemptions and justifications |
homeassistant/components/gios/manifest.json |
Declares the integration as Platinum-tier quality scale |
script/hassfest/quality_scale.py |
Removes GIOS from INTEGRATIONS_WITHOUT_QUALITY_SCALE_FILE and INTEGRATIONS_WITHOUT_SCALE exemption lists to enable quality scale validation |
|
Maybe rebasing will remove the codec complaints? |
mik-laj
force-pushed
the
gios-quality-scale-2
branch
2 times, most recently
from
f2e98a0 to
dc8d720
Compare
I did rebase twice and now CI is green. |
|
No. CI is still sad. 😞 but it looks unreleased. |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Let's put the comments in the file for the things we agree on and merge it and improve from there!
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done | ||
| config-flow: done |
There was a problem hiding this comment.
Let's limit the scope of the try block in the user step
| entity-event-setup: done | ||
| entity-unique-id: done | ||
| has-entity-name: done | ||
| runtime-data: done |
There was a problem hiding this comment.
No direct need to wrap the coordinator in a dataclass to store in the config entry
| comment: This integration does not require authentication. | ||
| test-coverage: done | ||
| # Gold | ||
| devices: done |
There was a problem hiding this comment.
Why do we actually cache the name in the entry data instead of using the up to date name?
There was a problem hiding this comment.
Previously, CONF_NAME was a user-configurable field, with "Home" used as the default value when the user did not provide a custom name.
With the changes introduced in #155762, the integration now always uses the value stored in CONF_NAME as the device name. The device name is no longer derived from live station data at runtime.
This makes CONF_NAME a required piece of persisted configuration data. To maintain backward compatibility - especially for downgrade scenarios where older Home Assistant versions still expect CONF_NAME to be present - we always populate CONF_NAME when creating the config entry.
There was a problem hiding this comment.
I wouldn't really consider downgrading a use case, but this is a nice food for thought
| status: exempt | ||
| comment: This integration does not have devices. | ||
| entity-category: done | ||
| entity-device-class: done |
There was a problem hiding this comment.
You can use the CO device class for the carbon monoxide sensor
| entity-category: done | ||
| entity-device-class: done | ||
| entity-disabled-by-default: done | ||
| entity-translations: done |
There was a problem hiding this comment.
I think we can remove the options state_attributes now as we discussed this before
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
I would recommend to have the happy flow as the first test, which can be merged with test_show_form.
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
The config flow tests are missing adding a duplicate entry
| reauthentication-flow: | ||
| status: exempt | ||
| comment: This integration does not require authentication. | ||
| test-coverage: done |
There was a problem hiding this comment.
I'd expect test_async_setup_entry to test the state of the mock config entry, instead of an wentity state
| reauthentication-flow: | ||
| status: exempt | ||
| comment: This integration does not require authentication. | ||
| test-coverage: done |
There was a problem hiding this comment.
test_availability doesn't really do what it says it does, and this is now already tested via the snapshot test
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
mik-laj
force-pushed
the
gios-quality-scale-2
branch
from
e9ba443 to
f629629
Compare
mik-laj
force-pushed
the
gios-quality-scale-2
branch
from
ff16e46 to
49aa5b4
Compare
Breaking change
Proposed change
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.To help with the load of incoming pull requests: