Skip to content

Removed default sensor configuration#12252

Merged
fabaff merged 3 commits intohome-assistant:devfrom
ChristianKuehnel:alpha_vantage_default_config
Feb 11, 2018
Merged

Removed default sensor configuration#12252
fabaff merged 3 commits intohome-assistant:devfrom
ChristianKuehnel:alpha_vantage_default_config

Conversation

@ChristianKuehnel
Copy link
Copy Markdown
Contributor

@ChristianKuehnel ChristianKuehnel commented Feb 8, 2018 •
edited by fabaff
Loading

Description:

As described in #12165, the sensor alpha vantage by default configures some smybols. This is removed now, instead a persistent_notification is shown if not symbols and no currencies are configured.

Related issue (if applicable): fixes #12165

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4618

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: alpha_vantage
    api_key: <your API key>
    # do not configure anything here to see the new persistent_notification

Checklist:

  • The code change is tested and works locally.

fabaff
fabaff requested changes Feb 8, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

You need to remove the unrelated commits aka those you want to introduce with #12249 from this PR.

Also, a description of the breaking change is required.

OttoWinter
OttoWinter reviewed Feb 8, 2018
View reviewed changes
Comment thread homeassistant/components/sensor/alpha_vantage.py Outdated
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Mostly unrelated, but why is there a space in 'TRY': 'mdi: currency-try', (after mdi:)... 🤔

Copy link
Copy Markdown
Contributor Author

@ChristianKuehnel ChristianKuehnel Feb 9, 2018

Choose a reason for hiding this comment

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

Done

@ChristianKuehnel ChristianKuehnel force-pushed the alpha_vantage_default_config branch from d33f7dd to 1548830 Compare February 9, 2018 19:57
@ChristianKuehnel ChristianKuehnel changed the title [WIP] sensor.alpha_vantage - fixed bug: removed default sensor configuration sensor.alpha_vantage - fixed bug: removed default sensor configuration Feb 9, 2018
@ChristianKuehnel
Copy link
Copy Markdown
Contributor Author

ChristianKuehnel commented Feb 9, 2018

@fabaff there should not be any breaking change here. That the two sensors were added by default is not described in the documentation. This is also the reason the users complained about it being a bug...

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Feb 9, 2018 •
edited
Loading

The docs says that there is a default and removing a default will break the configuration of users who are running with the configuration sample. For foreign_exchange it's indeed not mentioned.

MartinHjelmare
MartinHjelmare requested changes Feb 10, 2018
View reviewed changes
Comment thread homeassistant/components/sensor/alpha_vantage.py Outdated
symbols = config.get(CONF_SYMBOLS)
conversions = config.get(CONF_FOREIGN_EXCHANGE)

if (symbols is None or len(symbols) == 0) and \
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Feb 10, 2018

Choose a reason for hiding this comment

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

if not symbols and not conversions:

Copy link
Copy Markdown
Contributor Author

@ChristianKuehnel ChristianKuehnel Feb 10, 2018

Choose a reason for hiding this comment

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

fixed

@ChristianKuehnel ChristianKuehnel mentioned this pull request Feb 10, 2018
Merged
2 tasks
@ChristianKuehnel
Copy link
Copy Markdown
Contributor Author

ChristianKuehnel commented Feb 10, 2018

added PR for change to documentation

MartinHjelmare
MartinHjelmare approved these changes Feb 10, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@fabaff fabaff changed the title sensor.alpha_vantage - fixed bug: removed default sensor configuration Removed default sensor configuration Feb 11, 2018
@fabaff
Copy link
Copy Markdown
Member

fabaff commented Feb 11, 2018 •
edited
Loading

Breaking change:

The default of the alpha_vantage sensor was removed. Either a symbol (symbols:) or a foreign exchange (foreign_exchange:) must be configured, otherwise you will not get any data.

@fabaff fabaff merged commit ed1d6f1 into home-assistant:dev Feb 11, 2018
@balloob balloob mentioned this pull request Feb 22, 2018
Merged
@edif30 edif30 mentioned this pull request Feb 25, 2018
Closed
@ChristianKuehnel ChristianKuehnel deleted the alpha_vantage_default_config branch March 18, 2018 09:22
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@OttoWinter OttoWinter OttoWinter left review comments

@fabaff fabaff fabaff approved these changes

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

No one assigned

Labels

breaking-change cla-signed integration: alpha_vantage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Alpha Vantage is polling Bitcoin even though it is not configured

5 participants

@ChristianKuehnel @fabaff @MartinHjelmare @OttoWinter @homeassistant