core icon indicating copy to clipboard operation
core copied to clipboard

Scrape move yaml config to integration key

Open gjohansson-ST opened this issue 3 years ago • 7 comments

Breaking change

Loading Scrape via platform key has been deprecated. Please move your config directly onto the scrape key.

scrape:
  - resource: ...

Proposed change

Makes the following changes

Reference: https://github.com/home-assistant/core/pull/70476#issuecomment-1172509117

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] 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

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/23262

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] 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:

  • [x] 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.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] 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:

gjohansson-ST avatar Jul 02 '22 10:07 gjohansson-ST

Hey there @fabaff, mind taking a look at this pull request as it has been labeled with an integration (scrape) you are listed as a code owner for? Thanks! (message by CodeOwnersMention)

~~Documentation needs updating. Should merge https://github.com/home-assistant/home-assistant.io/pull/23253 first and then make new.~~ Completed

gjohansson-ST avatar Jul 02 '22 10:07 gjohansson-ST

and then make new. Merged! 👍

frenck avatar Jul 03 '22 20:07 frenck

Not sure what codecov is doing. It's compaining about all empty lines and docstrings

gjohansson-ST avatar Jul 10 '22 08:07 gjohansson-ST

Codecov is doing something wrong. Tests are 100% coverage

---------- coverage: platform linux, python 3.9.13-final-0 -----------
Name                                             Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------
homeassistant/components/scrape/__init__.py         50      0   100%
homeassistant/components/scrape/config_flow.py      24      0   100%
homeassistant/components/scrape/const.py             8      0   100%
homeassistant/components/scrape/sensor.py          116      0   100%
------------------------------------------------------------------------------
TOTAL                                              198      0   100%

gjohansson-ST avatar Sep 17 '22 13:09 gjohansson-ST

Hi @gjohansson-ST,

I was looking at the REST sensor earlier today (see #80543, #80544, #80546) and that is now using a DataCoordinator for sensor and binary_sensor platforms. Would it make sense to implement that right away as part of the migration, so we don't have a new breaking change so soon after the migration to the integration key?

Also, I am confused around the Config Flow tweaks that are in this PR. Surely those should be in a separate PR...

epenet avatar Oct 18 '22 15:10 epenet

@epenet Yes. Let's do a data coordinator right away. Don't think the config flow is with tweaks, it's pretty straight forward, but you're right. I will remove that and we do that in a separate PR.

gjohansson-ST avatar Oct 18 '22 18:10 gjohansson-ST

I think #81130 is the last preliminary PR I have in mind for scrape. After that we can focus again on the "integration YAML"

For the "integration YAML", I really think that we need to implement a two-level configuration:

scrape:
  - resource: https://example.com
    authentication: basic
    username: username
    password: password
    sensors:
      - name: sensor 1
        select: td
        index: 1
        unit_of_measurement: °C
      - name: sensor 2
        select: td
        index: 2
        unit_of_measurement: °C

The coordinator would be created and tested inside __init__.py, and then included in the data passed to sensor.py Then in sensor.py, we can create the coordinator for old config, and use the passed coordinator for new config.

epenet avatar Oct 28 '22 08:10 epenet

The coordinator would be created and tested inside __init__.py, and then included in the data passed to sensor.py Then in sensor.py, we can create the coordinator for old config, and use the passed coordinator for new config.

Worked on getting this on track yesterday but was a chunk to go through so if you have a look now when I published it should be pretty aligned with previous PR's.

Still requires cleanups and adapting the sensor tests which I will do next.

gjohansson-ST avatar Oct 28 '22 09:10 gjohansson-ST

Just a thought, could you add a comment here with the coverage result, and a small note about the coverage failure?

epenet avatar Oct 29 '22 09:10 epenet

Coverage 100% besides config_flow.py which is a left-over from previous PR where config flow was implemented and later reverted. Config flow intended to be implemented in follow-up PR so file therefore left as is.

---------- coverage: platform linux, python 3.9.15-final-0 -----------
Name                                             Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------
homeassistant/components/scrape/__init__.py         33      0   100%
homeassistant/components/scrape/config_flow.py      24     24     0%   2-121
homeassistant/components/scrape/const.py             9      0   100%
homeassistant/components/scrape/coordinator.py      17      0   100%
homeassistant/components/scrape/sensor.py           83      0   100%
------------------------------------------------------------------------------
TOTAL                                              166     24    86%

gjohansson-ST avatar Oct 29 '22 10:10 gjohansson-ST

Looks good to me. ~But I've probably been involved too closely so I'd like a second opinion.~

I think we can merge this and we can address comments in a follow-up PR.

epenet avatar Oct 30 '22 12:10 epenet