Add huawei_lte quality scale YAML #143347
Conversation
To keep track, one more item to go before bronze.
|
Hey there @fphammerle, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Please first fix the missing translations (in a new PR) so that the tests can pass: |
|
data_descriptions added in #143388. It's for the good, but I found it a bit surprising that
|
I fully agree with you (cc @joostlek) I encountered the same when adding quality scale for Shelly - #141062 (comment) I think this should be fixed to allow adding quality scall with these marked as |
|
They're optional in the code, but required to fulfil the rule, is that what you mean? |
Unless I was marking something wrong, assuming you are not ready to fulfill the rule and want to mark it as |
|
CI should not fall if you set it to todo |
There was a problem hiding this comment.
Let's put the things we agree on in the comments and let's get this merged and improve from there.
The integration can make use of some modern patterns which would increase the maintainability by a lot, while that is not explicitly covered by the quality scale, I would recommend to do that. I am available on Discord if you want to discuss things or want to challenge things :)
| @@ -0,0 +1,76 @@ | |||
| rules: | |||
There was a problem hiding this comment.
Should we clean up the entry unique id migration? I think at this time we can assume that everyone is running >2021.8 (and that if they decide to upgrade, they probably have bigger fish to fry)
There was a problem hiding this comment.
Yeah, I suppose that can go now.
| common-modules: done | ||
| config-flow-test-coverage: done | ||
| config-flow: done | ||
| dependency-transparency: done |
There was a problem hiding this comment.
The dependency is not built and published in a CI
There was a problem hiding this comment.
We don't actually know that though. huawei-lte-api and stringcase don't seem to be published from a public CI it seems, right, downgraded to TODO and added a note in aafc685. Also with that assumption, added a clarifying PR for the docs in home-assistant/developers.home-assistant#2656
Also, the item about building/publishing in a CI is a "should", not "must", in the rule documentation. There are two "must"s and two "should"s in the doc. Given the doc is all about the quality scale rule, I feel there should be a difference between "must" and "should". Or perhaps we can mark the item as done, but add a comment when a "should" item is not addressed?
There was a problem hiding this comment.
Publishing in a public CI is a must as with a project this size, that is the least we can do to make our dependencies more transparent
There was a problem hiding this comment.
In that case, it sounds that the documentation at https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/dependency-transparency/ needs to be updated to say "must" for all items, not "should" for some.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Thank you for the list of things to look into, will do sometime. There's a lot that can be improved in the integration, I know, but I've found it so hard to find reviewers for changes in this integration in general that TBH I haven't been that motivated to land too much work on it, nor do I have much time to invest in following the direction where HA codebase goes these days. It would be great to get the quality scale YAML in, because that's at least one way I can have a some kind of actual TODO list (even if incomplete) for the integration (because TODO/FIXME comments in code at least were not allowed by the linting config, and if I file issues for TODOs they tend to require herding or go stale). |
|
I'm marking this ready for review due per the perfect PR recommendations. The additional things suggested are very much appreciated, but as noted they're not strictly speaking in scope for adding the quality scale file and keeping the PR as small as possible to do that. |
|
Well, we can put things into the comments so we can keep track of them |
|
True, but putting them in comments here means
|
|
And in case by comments it was meant comments in the quality YAML and not in this PR, I'm not sure if that's the correct place either -- not all items here map to quality scale rules, and I'm not sure if it's appropriate to put arbitrary other TODOs there. I'm open to other opinions though. |
|
I mean the things related to the rules can be put in the comment of that rule, the rest can't and will live here |
|
So to make sure I understand, if we add the comments in the quality scale YAML that apply to items in it, then we're good to merge, and other comments made are just left here in this PR issue for reference after merge? |
|
Yes From a personal point of view, as this is about quality I would find something about it being graded without being migrated to a coordinator for example. But once we are crossing of things of the todolist this can just be an easy process :) |
|
plaese let me know if you think its ready because so far I don't know if you think its ready or not |
|
Pushing back to draft while I go through the comments directly related to the quality scale file, will mark as ready when I think it is. |
Proposed change
To keep track.
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: