Skip to content

Benadryl Social Pollen Count Sensor#24403

Closed
timmo001 wants to merge 2 commits into
home-assistant:devfrom
timmo001:pollen_count
Closed

Benadryl Social Pollen Count Sensor#24403
timmo001 wants to merge 2 commits into
home-assistant:devfrom
timmo001:pollen_count

Conversation

@timmo001
Copy link
Copy Markdown
Member

@timmo001 timmo001 commented Jun 8, 2019

Description:

Adds a new sensor platform for the pollen count.

image

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9583

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: pollen_benadryl

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.

@timmo001 timmo001 marked this pull request as ready for review June 8, 2019 10:12
@timmo001 timmo001 changed the title Benadryl Social Pollen Count 🌻 Benadryl Social Pollen Count Jun 8, 2019
@timmo001 timmo001 changed the title Benadryl Social Pollen Count Benadryl Social Pollen Count Sensor Jun 8, 2019
@frenck frenck mentioned this pull request Jun 8, 2019
9 tasks
if name is not None:
self.name = name
else:
self.name = DEFAULT_NAME
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.

Why pipe the name to PollenData and not to the entity? That way you wouldn't have to fetch it from PollenData each time you call update (which is kinda ugly)

def update(self):
"""Update Pollen Sensor."""
try:
response = requests.get(self._url)
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.

We generally don't want to use requests anymore (#23685) - Could you try converting this to aiohttp (API is mostly the same, and there are plenty of examples in this repo)

@Swamp-Ig
Copy link
Copy Markdown
Contributor

Swamp-Ig commented Jun 9, 2019

I have a few concerns regarding this one:

  • It's quite proprietary, which means the API is likely to change
  • It only covers the UK, nowhere else in the world
  • It doesn't hide the implementation details in a library
  • It's really just a wrapper around a single REST call.

My feeling is that this would be better served by a REST sensor, and not accepting this PR.

@timmo001
Copy link
Copy Markdown
Member Author

This is essentially an over-engineered rest sensor. Closing

@timmo001 timmo001 closed this Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants