Conversation
epenet
left a comment
There was a problem hiding this comment.
A few comments to get you started... In particular, try to go "type hint all the way"
|
Could we get this in 2021.12? |
There are currently 398 PRs in progress! You can also look at the backlog for new integrations: https://github.com/home-assistant/core/projects/5 |
| ) | ||
|
|
||
|
|
||
| class UptimeKumaBinarySensor(BinarySensorEntity, CoordinatorEntity): |
There was a problem hiding this comment.
Shouldn't this be implementing the available property as well? (If the monitored name isn't in the coordinator data anymore?)
There was a problem hiding this comment.
Doesn't an entity show as unavailable automatically if data is not reported for it?
| assert result | ||
| assert result.get("type") == data_entry_flow.RESULT_TYPE_FORM | ||
| assert result.get("step_id") == "user" | ||
| assert result.get("errors") == {"base": "cannot_connect"} |
There was a problem hiding this comment.
Please continue the tests where the user recovers and does complete successfully (to ensure there are no side-effect caused by this error).
There was a problem hiding this comment.
Is this required to get this merged? TBH, I don't really understand how to test things well. I mostly just addapted the existing tests from other integrations.
There was a problem hiding this comment.
where the user recovers and does complete successfully
Could you explain what you mean by 'recovers'? I'm not quite sure.
There was a problem hiding this comment.
Well, the form is shown again, so a user can correct their input and continue and complete the integration setup. That is what the tests need to do as well (to test and ensure that is the case).
|
Additionally, please resolve the existing merge conflict (I suggest to rebase the PR) and make sure the tests pass 👍 |
|
@frenck I'm not sure how to get this rebased. Could you help or point me in the right direction? |
|
Basically, you'd run a You'll run into a merge conflict you'll need to resolve, once resolve, run |
|
Ok, Thanks. I'll give it a try 👍 |
|
Hi @GitHub-Action, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
@frenck |
|
Hi @GitHub-Action, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
82d82d8 to
40f8d13
Compare
Yes, rebase again just with your commits. |
|
Sorry to waste your time, but i can't seem ti figure out how to rebase with only my commits. It seems to bring along all the other changes as well (It shows like 250 conflicts). |
|
Hello @meichthys, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |

Type of change
Checklist
toxto run on MacOS (what i use for development environment)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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: