Skip to content

Add config flow to nws and remove yaml configuration#34267

Merged
bdraco merged 7 commits intohome-assistant:devfrom
MatthewFlamm:nws_add_config_flow
Apr 16, 2020
Merged

Add config flow to nws and remove yaml configuration#34267
bdraco merged 7 commits intohome-assistant:devfrom
MatthewFlamm:nws_add_config_flow

Conversation

@MatthewFlamm
Copy link
Copy Markdown
Contributor

@MatthewFlamm MatthewFlamm commented Apr 15, 2020

Breaking change

YAML config longer supported for NWS integration, configuration is now done though UI. Two entities are now created for each configured entry, one for daynight and one for hourly. The mode option is no longer needed and name option is no longer supported.

Proposed change

Brings NWS in line with ADR0010. This would be best to be done before 0.109 since NWS just switched to a component YAML configuration for 0.109. If implemented for 0.109, we avoid having two breaking changes.

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

Comment thread homeassistant/components/nws/__init__.py
Comment thread homeassistant/components/nws/config_flow.py Outdated
Comment thread homeassistant/components/nws/__init__.py Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 15, 2020

@MatthewFlamm Looks good, just a few minor comments.

I'm testing it now

@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

How do I clean up the breaking change sections with #31398 so they make sense?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 15, 2020

How do I clean up the breaking change sections with #31398 so they make sense?

Once this merges we can delete the Breaking change section in #31398 since this PR supersedes it and it will never be released.

Comment thread homeassistant/components/nws/.translations/en.json Outdated
Comment thread homeassistant/components/nws/.translations/en.json Outdated
Comment thread homeassistant/components/nws/.translations/en.json
Comment thread homeassistant/components/nws/strings.json Outdated
Comment thread homeassistant/components/nws/.translations/en.json Outdated
@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

You can tell I didn't carefully consider the information in the strings.json. These are all good suggestions and will clean them up later.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 15, 2020

Testing looks good

@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

I have pushed @balloob suggestion to move title under root, but it doesn't work in my testing. Am I doing something wrong? It works on my end the way it was originally. Can someone else test?

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 16, 2020

The update to the frontend for that change (home-assistant/frontend#5546) has not been released yet as a dev release on the backend.

@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

I updated the breaking change section here so that it also encompasses the relevant pieces of the previous breaking change note. I see a lint error that didn't appear on my local dev environment that I will clear up tomorrow, and I'd like to test the strings.json change when the frontend version is updated on the backend.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 16, 2020

@MatthewFlamm Other then the pylint homeassistant/components/nws/config_flow.py:65:8: C0103: Variable name "DATA_SCHEMA" doesn't conform to snake_case naming style (invalid-name) this looks good to go.

MatthewFlamm and others added 3 commits April 16, 2020 13:44
Co-Authored-By: J. Nick Koston <nick@koston.org>
Co-Authored-By: J. Nick Koston <nick@koston.org>
@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

Not sure what is going on with the CI, but rebased on dev to see if the unrelated test fail will pass and CI will show up correctly.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 16, 2020

retesting...

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 16, 2020

Config flow title isn't working but I suspect that is due to other changes outside of this.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 16, 2020

Config flow title isn't working but I suspect that is due to other changes outside of this.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 16, 2020

LGTM

Verified I could add and remove the integration and the data looks sane

@bdraco bdraco merged commit 6d812bd into home-assistant:dev Apr 16, 2020
@MatthewFlamm
Copy link
Copy Markdown
Contributor Author

Thanks for the quick and thorough review @bdraco. According to above the title needs frontend to be updated to work. I will definitely test when it gets applied at least in the beta release.

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

5 participants