Skip to content

Fix Geizhals integration, use geizhalscrawler (#38123)#48593

Closed
neubi4 wants to merge 1 commit intohome-assistant:devfrom
neubi4:fix-geizhals
Closed

Fix Geizhals integration, use geizhalscrawler (#38123)#48593
neubi4 wants to merge 1 commit intohome-assistant:devfrom
neubi4:fix-geizhals

Conversation

@neubi4
Copy link
Copy Markdown

@neubi4 neubi4 commented Apr 1, 2021

Breaking change

Proposed change

Use the updated geizhalscrawler package as suggested in #38123 for the geizhals sensor (https://www.home-assistant.io/integrations/geizhals/) instead of the not working old package. The package "geizhals" is in read-only state and not maintained anymore.

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
sensor:
  - platform: geizhals
    name: geizhals-35WN73A-B
    product_id: 2370355
    description: "LG 35WN73A-B, 35"
    locale: "DE"

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

To help with the load of incoming pull requests:

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @neubi4,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant probot-home-assistant Bot added bugfix dependency Pull requests marked as a dependency upgrade integration: geizhals small-pr PRs with less than 30 lines. labels Apr 1, 2021
"name": "Geizhals",
"documentation": "https://www.home-assistant.io/integrations/geizhals",
"requirements": ["geizhals==0.0.9"],
"requirements": ["geizhalscrawler==0.1"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This packages uses webscraping to gather information. This is not allowed and violated architectural decision record 0004 (ADR-0004).

See: https://github.com/home-assistant/architecture/blob/master/adr/0004-webscraping.md

Upstream code sample:

        # parse prices
        self.device.prices = []
        for tmp in soup.select('div.offer__price .gh_price'):
            matches = re.search(_REGEX, tmp.string)
            raw = '{}.{}'.format(matches.group(1),
                                 matches.group(2))
            self.device.prices += [float(raw)]

https://github.com/timherrm/geizhalscrawler/blob/60fe6b1db9344412541ce95235da5f8e61ff9a3a/geizhalscrawler/geizhalscrawler.py#L82

@frenck frenck closed this Apr 1, 2021
@frenck frenck mentioned this pull request Apr 1, 2021
21 tasks
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix cla-signed dependency Pull requests marked as a dependency upgrade integration: geizhals small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Geizhals Integration

3 participants