Improve Huawei LTE SSDP inclusion #85572
Conversation
home-assistant
bot
added
bugfix
cla-signed
has-tests
integration: huawei_lte
small-pr
|
Hey there @fphammerle, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
scop
force-pushed
the
scop-huawei-lte-ssdp-api-probe
branch
from
17d6f43
to
cdbeb12
Compare
|
I marked this as draft since the tests are failing |
scop
changed the title
|
It would be great to have more smaller PRs than one that does many things, i.e., separate version bumps especially when it bumps over multiple releases. If possible, I'd avoid the version bump in this PR and handle it on its own. That being said, if it's necessary to have them in one PR, it would be great to have a link to the changes so anyone doing a review can easily perform a review on those. |
I know. No problem doing that here if there are reviewers available. Historically more/smaller PR's has not helped with that in my experience, on the contrary more often it has been the case that reviewers have asked more, not strictly in-scope changes during their review. It's quite impossible to know beforehand. But just say the word and I'll split this up.
I added a link to the complete diff in the description yesterday, Salamek/huawei-lte-api@1.6.7...1.6.11 |
|
Note to self, mostly: if we split this up, I'll make this one stack on top of the huawei-lte-api dep bump PR and not rework the code to work independently of it. huawei-lte-api changes in another form: https://github.com/Salamek/huawei-lte-api/releases (no release notes for 1.6.11, seems it's a brown paper bag fixup one). |
|
Case in point for "reviewers asking more" is #84019 The whole PR was initially created in part to address a (post-approval) comment not in scope of the original PR and unchanged by it in #83904 (comment) When it entered review, thankfully a reviewer emerged quickly, however asking some more changes that were ok with the general theme of the PR, but weren't strictly necessary and caused the diff to grow substantially: #84019 (comment). I made those changes a few days later, dropped a note about them, then pinged back a week later. It's now been 3 weeks since my last ping. Sure, holiday season and all, but in principle. |
|
Split the lib upgrade out already in #85669 to save one possible roundtrip, will rework this on top of it if we decide to merge that first. |
|
Another case (unrelated to huawei_lte) where a reviewer asks for quite substantially more than what would be required for the scope of the submitted PR as such is at #68718 (comment) -- that stuff could very well be done in a followup PR. |
I feel your frustration as I have also been on the receiving end of hung PRs. The problem is that there are not enough knowledgeable reviewers, and having more changes means in practice more time that needs to be spent on reviewing the code, which is painful especially if one doesn't have a test device. I personally find it fine to remind an earlier reviewer just in case they have forgotten (or clicked the ignore) after some time has gone by, but not everyone may agree on that... :-) Alas, there are no easy solutions for the lack of willing reviewers. I created home-assistant/developers.home-assistant#1553 some time back as a reminder to start collecting items to check to make the code reviews easier in hopes of making it easier to offload the review burden. But as it often happens, other higher-priority things came up... Also, as a casual reviewer, this would require quite a bit of effort from my side to go through historical code reviews (and the relevant parts in the docs...) to collect checklist items that oftentimes come up (either pre or post-merge), something which may turn out to be a total waste of time without buy-in from the core developers. |
More or less as expected, the loosening of SSDP/UPnP data matches done in #81643 started to yield false positives, as in #85402. Coming up with robust matches solely based on the SSDP/UPnP data still does not seem possible, so keep the matches as loose as they were made, but additionally invoke a probe request on the API to determine if the device looks like a supported one.
Prevents throwaway probes for discoveries already in progress.
scop
force-pushed
the
scop-huawei-lte-ssdp-api-probe
branch
from
d49eee2
to
ef83330
Compare
scop
changed the title
|
Rebased, summary + description and list of fixed bugs updated. |
* Probe Huawei LTE API for device support on SSDP match More or less as expected, the loosening of SSDP/UPnP data matches done in #81643 started to yield false positives, as in #85402. Coming up with robust matches solely based on the SSDP/UPnP data still does not seem possible, so keep the matches as loose as they were made, but additionally invoke a probe request on the API to determine if the device looks like a supported one. * Probe only after unique id checks Prevents throwaway probes for discoveries already in progress. * Fix SSDP result URL test, add missing assert on it
Proposed change
More or less as expected, the loosening of SSDP/UPnP data matches done in #81643 started to yield false positives, as in #85402.
Coming up with robust matches solely based on the SSDP/UPnP data still does not seem possible, so keep the matches as loose as they were made, but additionally invoke a probe request on the API to determine if the device looks like a supported one.
Type of change
Additional information
Checklist
black --fast 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: