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.

Sign up for GitHub

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

Jump to bottom

Fix geizhals price parsing #15990

Merged
merged 10 commits into from
Aug 27, 2018
Merged

Fix geizhals price parsing #15990

merged 10 commits into from
Aug 27, 2018

Conversation

JulianKahnert
Copy link
Contributor

@JulianKahnert JulianKahnert commented Aug 15, 2018 •
edited
Loading

Description:

BREAKING CHANGE: The configuration changes from

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985
    description: "Bose QC35"
    domain: 'geizhals.de'
    regex: '\D\s(\d*)[\,|\.](\d*)'

to:

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985
    description: "Bose QC35"
    locale: "DE"

Possible locale values are: AT, EU, DE, UK or PL

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@ghost ghost added the in progress label Aug 15, 2018
@ghost ghost assigned fabaff Aug 16, 2018
MartinHjelmare
MartinHjelmare requested changes Aug 16, 2018
View reviewed changes
Copy link
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.

The GeizParser class should be broken out into standalone library published on pypi.

https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#6-communication-with-devices-services

balloob
balloob requested changes Aug 16, 2018
View reviewed changes
homeassistant/components/sensor/geizhals.py Outdated
raw = soup.find_all('span', attrs={'itemprop': 'name'})
self.device_name = raw[1].string
# Parse name
raw = soup.find('h1', attrs={'class': 'gh-headline'})
Copy link
Member

Choose a reason for hiding this comment

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

This should be extracted from Home Assistant and into a separate Python library. We will not be able to accept any changes to this platform until this is fixed.

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 your comments @MartinHjelmare and @balloob ! I will create a separate library for that.

@JulianKahnert JulianKahnert changed the title fix geizhals price parsing WIP: fix geizhals price parsing Aug 16, 2018
@JulianKahnert JulianKahnert mentioned this pull request Aug 16, 2018
Merged
2 tasks
@JulianKahnert JulianKahnert changed the title WIP: fix geizhals price parsing fix geizhals price parsing Aug 16, 2018
@JulianKahnert
Copy link
Contributor Author

I found a bug in the geizhals Library. Please do not merge it yet!

@MartinHjelmare MartinHjelmare changed the title fix geizhals price parsing WIP: Fix geizhals price parsing Aug 17, 2018
@MartinHjelmare
Copy link
Member

Before we merge we should also add a paragraph in the PR description about the breaking change, since config options changed.

@JulianKahnert
Copy link
Contributor Author

The Geizhals library was fixed in JulianKahnert/geizhals@5401a94 and the HA component was fixed in b949fc5.

@MartinHjelmare, I added a description of the breaking change in the PR description.

@JulianKahnert JulianKahnert changed the title WIP: Fix geizhals price parsing Fix geizhals price parsing Aug 17, 2018
MartinHjelmare
MartinHjelmare reviewed Aug 17, 2018
View reviewed changes
Copy link
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.

We should initialize self.device in init.

@JulianKahnert
Copy link
Contributor Author

Thanks @MartinHjelmare for your reply. You are absolutly right! I added this in 828d556

@JulianKahnert
Copy link
Contributor Author

Hey @MartinHjelmare , can you merge this?

@MartinHjelmare
Copy link
Member

There seems to be a problem installing the geizhals library. The travis job failed:
https://travis-ci.org/home-assistant/home-assistant/jobs/417445670#L727

It looks like the package is imported in setup.py. That won't work since dependencies aren't installed at that time.
https://github.com/JulianKahnert/geizhals/blob/master/setup.py#L2

MartinHjelmare
MartinHjelmare requested changes Aug 23, 2018
View reviewed changes
homeassistant/components/sensor/geizhals.py Outdated
self.data = GeizParser(product_id, domain, regex)
self._state = None
self.product_id = product_id
self.device = Device()
Copy link
Member

Choose a reason for hiding this comment

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

Please use another attribute name. Entity device is a property now after a recent PR.

@balloob
Copy link
Member

balloob commented Aug 24, 2018

You still have an installation issue with your lib. You cannot import your own lib inside setup.py because it needs to read setup.py to see what your dependencies are and install them.

@JulianKahnert
Copy link
Contributor Author

JulianKahnert commented Aug 24, 2018 •
edited
Loading

Hi there, I'm sorry for wasting your time! d20403d should fix the problem.

houndci-bot
houndci-bot reviewed Aug 24, 2018
View reviewed changes
homeassistant/components/sensor/geizhals.py Outdated

add_entities([Geizwatch(name, description, product_id, domain, regex)],
True)
add_devices([Geizwatch(name, description, product_id, domain)],
Copy link

Choose a reason for hiding this comment

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

undefined name 'add_devices'

@balloob balloob merged commit 2e9db1f into home-assistant:dev Aug 27, 2018
@ghost ghost removed the in progress label Aug 27, 2018
mbennett pushed a commit to mbennett/home-assistant that referenced this pull request Aug 27, 2018
@JulianKahnert
Fix geizhals price parsing (home-assistant#15990)
b7803bc 
* fix geizhals price parsing

* Fix lint issue

* switch to the geizhals pypi package

* throttle updates

* update geizhals version

* initialize empty device

* minor changes to trigger another TravisCI test

* device => _device

* bump geizhals version
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
@JulianKahnert
Fix geizhals price parsing (home-assistant#15990)
761800e 
* fix geizhals price parsing

* Fix lint issue

* switch to the geizhals pypi package

* throttle updates

* update geizhals version

* initialize empty device

* minor changes to trigger another TravisCI test

* device => _device

* bump geizhals version
@balloob balloob mentioned this pull request Sep 17, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare requested changes

@balloob balloob balloob approved these changes

Assignees

@fabaff fabaff

Labels
breaking-change cla-signed integration: geizhals small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JulianKahnert @MartinHjelmare @balloob @houndci-bot @fabaff @homeassistant