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

Conversation

@michaeldavie
Copy link
Contributor

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 michaeldavie/env_canada#1)

Sensor

  • Remove optional monitored_conditions from configuration, create sensors for all provided data
  • Remove optional name from configuration
  • Add optional language to configuration (addresses michaeldavie/env_canada#2)
  • Use unique_id for each sensor (addresses michaeldavie/env_canada#3)
  • Remove hardcoded labels and units, use values supplied by env_canada instead (addresses michaeldavie/env_canada#5)
  • Change refresh period to 1 minute

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',

This comment has been minimized.

Copy link
@frenck

frenck Jul 1, 2019

Member

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

This comment has been minimized.

Copy link
@michaeldavie

michaeldavie Jul 1, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@balloob

balloob Jul 1, 2019

Member

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

This comment has been minimized.

Copy link
@michaeldavie

michaeldavie Jul 2, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@frenck

frenck Jul 2, 2019

Member

That is just working around the original concern raised!?

This comment has been minimized.

Copy link
@balloob

balloob Jul 11, 2019

Member

unique ID != entity_id

This comment has been minimized.

Copy link
@michaeldavie

michaeldavie Jul 11, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@balloob

balloob Jul 12, 2019

Member

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

This comment has been minimized.

Copy link
@michaeldavie

michaeldavie Jul 12, 2019

Author Contributor

Thanks for clarifying, I've switched this over.

@michaeldavie michaeldavie force-pushed the michaeldavie:ec_updates branch 2 times, most recently from c943bdf to 4949354 Jul 2, 2019

@michaeldavie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

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 force-pushed the michaeldavie:ec_updates branch from f0ad7b5 to c0b707a Jul 6, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

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 michaeldavie force-pushed the michaeldavie:ec_updates branch from 96222ab to b92bed9 Jul 10, 2019

@michaeldavie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

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.

@michaeldavie michaeldavie force-pushed the michaeldavie:ec_updates branch from b92bed9 to 02e97a3 Jul 12, 2019

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'):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 12, 2019

Member

Are we ok with this config option? @balloob?

This comment has been minimized.

Copy link
@michaeldavie

michaeldavie Jul 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@pvizeli

pvizeli Jul 13, 2019

Member

On sensor, yes

This comment has been minimized.

Copy link
@pvizeli

pvizeli Jul 13, 2019

Member

But yeah, we should add a core language config flag

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 homeassistant/components/environment_canada/sensor.py Outdated

michaeldavie added some commits Jul 1, 2019

michaeldavie added some commits Jul 10, 2019

@michaeldavie michaeldavie force-pushed the michaeldavie:ec_updates branch from 3f8533f to 9ce3752 Jul 13, 2019

@pvizeli pvizeli merged commit a147a18 into home-assistant:dev Jul 13, 2019

9 checks passed

CI Build #20190713.16 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pytlint) FullCheck Pytlint succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python35) Tests PyTest Python35 succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA

@michaeldavie michaeldavie deleted the michaeldavie:ec_updates branch Jul 13, 2019

@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.
You can’t perform that action at this time.