search

dlarrick / hass-kumo

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

Clean up whitespace and stylistic changes #19

Closed omriasta closed 4 years ago

omriasta commented 4 years ago

This should be all the whitespace and stylistic changes. The only other change you have here is that homeassistant stated that ClimateDevice has been deprecated and should be replaced with ClimateEntity which I did. You will also have to edit init.py to work with the config_flow that is why they provide the examples which I can put in when I make the PR for the config flow files. Let me know if this is how you want it and I should do the next?

omriasta commented 4 years ago

I have to look at the init again later today. The rest were corrections from the dev environment. The comments need to be grammatically phrased as a command for some reason. The kumo cloud setup has a space between """ and Fetch. The dev environment would not let me proceed with that

omriasta commented 4 years ago

My bad I didn't put the right file in there. I think I updated it in the "fix init file" above....hopefully I did it right.... Git is hard :)

dlarrick commented 4 years ago

When I try to start up HomeAssistant with your branch I get: 2020-05-17 14:59:09 ERROR (MainThread) [homeassistant.setup] Unable to prepare setup for platform kumo.climate: Platform not found (cannot import name 'ClimateEntity' from 'homeassistant.components.climate' (/home/doug/Projects/hass/homeassistant/lib/python3.8/site-packages/homeassistant/components/climate/__init__.py)).

I know you said ClimateDevice is deprecated but I see no evidence of other 'climate' integrations in the source tree moving away from it. If I replace both instances of "ClimateEntity" with "ClimateDevice" as it used to be, it at least starts up and finds my units.

Where did you see that ClimateDevice is deprecated?

omriasta commented 4 years ago

I received the message in the same dev environment using the same test that gave me the other errors.

On Sun, May 17, 2020, 3:35 PM dlarrick notifications@github.com wrote:

When I try to start up HomeAssistant with your branch I get: 2020-05-17 14:59:09 ERROR (MainThread) [homeassistant.setup] Unable to prepare setup for platform kumo.climate: Platform not found (cannot import name 'ClimateEntity' from 'homeassistant.components.climate' (/home/doug/Projects/hass/homeassistant/lib/python3.8/site-packages/homeassistant/components/climate/init.py)).

I know you said ClimateDevice is deprecated but I see no evidence of other 'climate' integrations in the source tree moving away from it. If I replace both instances of "ClimateEntity" with "ClimateDevice" as it used to be, it at least starts up and finds my units.

Where did you see that ClimateDevice is deprecated?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dlarrick/hass-kumo/pull/19#issuecomment-629848981, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2GPSFBIHP3UDQOO5PV2D3RSA4ARANCNFSM4NC5P4IQ .

dlarrick commented 4 years ago

I can't take this change if it doesn't actually work. Please test your changes before making PRs.

omriasta commented 4 years ago

The error I got was climatedevice is deprecated, modify to extend climateentity It might be that this is a change in 0.110. see documentation here: https://developers.home-assistant.io/docs/entity_climate/

I tested in the dev environment which is currently on version 0.110, and it did work. It could be that this is a new change

On Sun, May 17, 2020, 3:44 PM dlarrick notifications@github.com wrote:

I can't take this change if it doesn't actually work. Please test your changes before making PRs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dlarrick/hass-kumo/pull/19#issuecomment-629850089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2GPSF4PNLEOQ2GGK2X3CTRSA5AHANCNFSM4NC5P4IQ .

dlarrick commented 4 years ago

Here's the relevant developer blog entry: https://developers.home-assistant.io/blog

Looks like this is indeed coming in 0.110. If you can fix up your PR so it does work I can take this now; otherwise I will likely need to do something in the 0.111 timeframe.

omriasta commented 4 years ago

OK, can you please test this though, pretty sure I got everything back as needed but didn't get a chance to test now since the dev environment won't let me load it this way.

dlarrick commented 4 years ago

This one starts up OK. I'll take a closer look in the morning and merge it unless I see anything else concerning.

omriasta commented 4 years ago

Cool, let me know if there is anything else I can do?

On Sun, May 17, 2020, 7:34 PM dlarrick notifications@github.com wrote:

This one starts up OK. I'll take a closer look in the morning and merge it unless I see anything else concerning.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dlarrick/hass-kumo/pull/19#issuecomment-629878931, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2GPSCPART6PXXNESSU5FLRSBYBRANCNFSM4NC5P4IQ .

dlarrick commented 4 years ago

I'm not going to make a new release just for cosmetic source code changes but it'll be rolled up in whatever goes out next.

omriasta commented 4 years ago

Great, I already sent a rough preliminary PR for the config flow as well...