Skip to content

Some small Monoprice fixes#35064

Closed
OnFreund wants to merge 4 commits intohome-assistant:devfrom
OnFreund:monoprice_stuff
Closed

Some small Monoprice fixes#35064
OnFreund wants to merge 4 commits intohome-assistant:devfrom
OnFreund:monoprice_stuff

Conversation

@OnFreund
Copy link
Copy Markdown
Contributor

@OnFreund OnFreund commented May 2, 2020

Proposed change

  1. Fix unloading of config entry (by unsubscribing to the update listener).
  2. Use suggested value (instead of default) for source names, so they can be easily removed in the options flow.
  3. Try to automatically detect zones on first run.
  4. Don't call update() before adding to the entity registry on subsequent runs, to speed boot up and avoid warnings.

Also added myself as a codeowner since over the past few weeks I've basically re-written it.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

Hey there @etsinko, mind taking a look at this pull request as its been labeled with a integration (monoprice) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@MartinHjelmare
Copy link
Copy Markdown
Member

If you split this PR into five PRs reviews will be faster.

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 2, 2020

@MartinHjelmare thanks. I understand and appreciate the logic - but most of these are either really small (1, 2, code owners), or related (3 & 4 go together). Is it worth the extra pipelines, etc? If it is, I'll split it up a bit.

@MartinHjelmare
Copy link
Copy Markdown
Member

It's always worth it to make PRs smaller. Smaller PRs skip the line. Smaller PRs are easier to review so less risk of bugs. Smaller PRs have less things to talk about so the review iteration will be shorter.

@OnFreund OnFreund marked this pull request as draft May 3, 2020 05:01
@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 3, 2020

OK, here's the first small PR in this series: #35107

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 3, 2020

Two more:
#35111
#35112

That last one will be hard to merge before #35112, so I'll wait for that one

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 3, 2020

The final PR:
#35127

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented May 3, 2020

Since this is all covered by smaller PRs now, and only one of those hasn't been merged yet, I'm closing this, in hope that #35127 gets reviewed and merged soon.

@OnFreund OnFreund closed this May 3, 2020
@lock lock Bot locked and limited conversation to collaborators May 6, 2020
@OnFreund OnFreund deleted the monoprice_stuff branch May 26, 2020 09:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants