Skip to content

Fix default elkm1 temp units#34274

Merged
balloob merged 4 commits intohome-assistant:devfrom
bdraco:elkm1_fix_temp_units
Apr 16, 2020
Merged

Fix default elkm1 temp units#34274
balloob merged 4 commits intohome-assistant:devfrom
bdraco:elkm1_fix_temp_units

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Apr 15, 2020

Proposed change

The yaml config uses cv.temperature_unit which mutates "F" -> "°F". The config flow conversion didn't contemplate that. We need the units in "°F" format for the config flow.

The default unit was "F" but the value needed to be "°F"

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

Example entry for configuration.yaml:

# Example configuration.yaml

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

The default units was "F" but the value needed to be
"°F"
@bdraco bdraco changed the title Fix elkm1 temp units Fix default elkm1 temp units Apr 15, 2020
@bdraco bdraco linked an issue Apr 16, 2020 that may be closed by this pull request
@bdraco bdraco marked this pull request as ready for review April 16, 2020 16:26
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Apr 16, 2020

Should be tagged for 0.108.6 if we do one

@bdraco bdraco added this to the 0.108.6 milestone Apr 16, 2020
@balloob balloob merged commit 9760957 into home-assistant:dev Apr 16, 2020
balloob pushed a commit that referenced this pull request Apr 17, 2020
@balloob balloob mentioned this pull request Apr 17, 2020
config = {"temperature_unit": conf[CONF_TEMPERATURE_UNIT]}
temperature_unit = TEMP_FAHRENHEIT
if conf[CONF_TEMPERATURE_UNIT] in (BARE_TEMP_CELSIUS, TEMP_CELSIUS):
temperature_unit = TEMP_CELSIUS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to implement a config entry migration handler to update the config entry data to the correct state before the integration is set up.

See eg:

async def async_migrate_entry(hass: HomeAssistantType, config_entry: ConfigEntry):
"""Migrate config entry to new version."""
if config_entry.version == 1:
options = config_entry.options
recipient = options.get(CONF_RECIPIENT)
if isinstance(recipient, str):
options[CONF_RECIPIENT] = [x.strip() for x in recipient.split(",")]
config_entry.version = 2
hass.config_entries.async_update_entry(config_entry, options=options)
_LOGGER.info("Migrated config entry to version %d", config_entry.version)
return True

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will look at cleaning this up once everything settles a bit more.

@lock lock Bot locked and limited conversation to collaborators Apr 18, 2020
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.

elkm1 climate entity for thermostats seems broken in 0.108.3 release

4 participants