search

dlarrick / hass-kumo

Home Assistant module interfacing with Mitsubishi mini-split units
MIT License
97 stars 21 forks source link

Reformatting with black to prepare for submitting with hass core #51

Closed danielgoepp closed 3 years ago

danielgoepp commented 3 years ago

I would like to assist in the process of getting this code accepted and merged into code. I'm no expert in this process, but am willing to put in some time to try to figure it out. I've really enjoyed using this integration, so I hope I can help.

First was to get it formatted correctly, which is what this request is about. As per:

https://developers.home-assistant.io/docs/development_guidelines

Also as per the guidelines I will try to keep my commits small and specific to the one thing being addressed.

Next, I'm going to review the other requirements. Please let me know if you are willing to go through this process. This is in regards to the ticket I created last year:

https://github.com/dlarrick/hass-kumo/issues/41

danielgoepp commented 3 years ago

I also ran isort as per the style guidelines

dlarrick commented 3 years ago

Sorry for the delay in replying. Super busy with my day job right now. I promise to take a look thru this PR soon, probably this coming weekend.

My ideal situation for getting this component merged to HA proper is for someone to run point with the HA folks and make stylistic changes as needed. If you're willing to be that person I'm thrilled. I'm happy to review code and approve PRs, and lend my advice & consulting for more significant changes. But see "day job" comment above.

There are a few areas where I think the integration could use some improvement:

  1. Full documentation, of course. This is likely required for merge to HA
  2. Better management / UI for "prefer_cache". As the recent service outage showed, for the most part users are best off running with prefer_cache true. But if you're not using static addressing (or static DHCP) and the IP address changes, or you add an indoor unit to your setup, you're going to need to refresh the cache at some point, and it's not that easy to do.
  3. Related, let people supply override IP address for their units. Sometimes it can be days before the cache file picks up changes.
  4. Understand why "Sometimes it can be days before the cache file picks up changes" is true, and offer advice in the documentation on how to get it to update.
danielgoepp commented 3 years ago

Yeah, no worries at all. I didn't expect this to get approved right away :) And yes, I'm willing to run point. I can't promise anything about the timeline, but I would really like to see this component merged into core. I've also not done the process before, so I will be learning as I go. I just know that these two code formatting requirements are the first things listed, so I figured I would get the ball rolling. I think it will be an iterative process, as per the recommendation of guide. it's easier to review and approve small changes. Also, to get the initial PR approved, as much of an MVP as possible, so we should make sure any code that isn't working or is not necessary is cleaned out. I hope to do a little bit each weekend, or every couple weekends.

dlarrick commented 3 years ago

I merged a patch off of this PR with another unrelated change (bumping the pykumo version) so this PR should be considered merged. Closing. Feel free to reopen if I missed something.

danielgoepp commented 3 years ago

Awesome, thanks you @dlarrick!