Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Environment Canada platforms #24884

Merged
merged 12 commits into from
Jul 13, 2019
Merged

Conversation

michaeldavie
Copy link
Contributor

@michaeldavie michaeldavie commented Jul 1, 2019

Description:

External Library

  • Bump env_canada to 0.0.18
  • Split classes into separate modules to remove unnecessary imports (e.g. numpy for conditions)
  • Return data and labels in either English (default) or French
  • Improve processing of severe weather alerts
  • Separate metadata from conditions
  • Add forecast text summary to conditions (addresses You should add the text summaries michaeldavie/env_canada#1)

Sensor

Weather

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9747

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: environment_canada
    language: french

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ghost ghost assigned michaeldavie Jul 1, 2019
@michaeldavie michaeldavie changed the title Add French support to Environment Canada sensor Add French support, forecast period, and text summary to Environment Canada sensor Jul 1, 2019

MIN_TIME_BETWEEN_UPDATES = datetime.timedelta(minutes=10)

SENSOR_TYPES = {
'temperature': {'name': 'Temperature',
'temperature': {'english': 'Temperature',
Copy link
Member

Choose a reason for hiding this comment

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

It feels counterproductive to me to put translations, hardcoded, in the backend.
A user can customize entities if he wants to, right?

Copy link
Contributor Author

@michaeldavie michaeldavie Jul 1, 2019

Choose a reason for hiding this comment

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

Yes, they can. In this case the source data is available in both English and French, and I'd like a user to be able to select one of the two languages and have both the labels and the underlying data match. I'm not sure that hardcoding two sets of labels is that different from hardcoding one.

Is there a way to leverage the user selection for the front end language to drive this?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't merge this, as translations are not stored inside the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I've moved the labels out to the env_canada library, which makes things cleaner. Please let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

That is just working around the original concern raised!?

Copy link
Member

Choose a reason for hiding this comment

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

unique ID != entity_id

Copy link
Contributor Author

@michaeldavie michaeldavie Jul 11, 2019

Choose a reason for hiding this comment

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

Are you saying that I should use something unique in the entity_id, e.g. sensor.env_canada.temperature? I used to have this and removed it based on a comment on the original PR for this platform: #21110 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have given more context. Thanks Martin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying, I've switched this over.

@michaeldavie michaeldavie force-pushed the ec_updates branch 2 times, most recently from c943bdf to 4949354 Compare July 6, 2019 18:06
@michaeldavie
Copy link
Contributor Author

This update to the env_canada library also includes a lot of enhancements related to alerts for severe weather, so it would be great to get it merged. Please let me know if there are any outstanding concerns.

@michaeldavie michaeldavie changed the title Add French support, forecast period, and text summary to Environment Canada sensor Updates to Environment Canada platforms Jul 6, 2019
@michaeldavie
Copy link
Contributor Author

I've updated the description to better describe the included changes.

@MartinHjelmare MartinHjelmare changed the title Updates to Environment Canada platforms Update Environment Canada platforms Jul 7, 2019
@michaeldavie
Copy link
Contributor Author

Should this be marked as a breaking change, since the entity_id values and the configuration are changing? I've updated the documentation to reflect the changes.

vol.Required(CONF_MONITORED_CONDITIONS, default=list(SENSOR_TYPES)):
vol.All(cv.ensure_list, [vol.In(SENSOR_TYPES)]),
vol.Optional(CONF_NAME): cv.string,
vol.Required(CONF_LANGUAGE, default='english'):
Copy link
Member

@MartinHjelmare MartinHjelmare Jul 12, 2019

Choose a reason for hiding this comment

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

Are we ok with this config option? @balloob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to pull this value from the front end configuration, if that's possible.

Copy link
Member

Choose a reason for hiding this comment

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

On sensor, yes

Copy link
Member

Choose a reason for hiding this comment

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

But yeah, we should add a core language config flag

homeassistant/components/environment_canada/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/environment_canada/sensor.py Outdated Show resolved Hide resolved
@pvizeli pvizeli merged commit a147a18 into home-assistant:dev Jul 13, 2019
@michaeldavie michaeldavie deleted the ec_updates branch July 13, 2019 21:08
@lock lock bot locked and limited conversation to collaborators Jul 14, 2019
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.

None yet

6 participants