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.

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

Add flume support #27235

Merged
merged 30 commits into from Nov 23, 2019
Merged

Add flume support #27235

merged 30 commits into from Nov 23, 2019

Conversation

ChrisMandich
Copy link
Contributor

@ChrisMandich ChrisMandich commented Oct 5, 2019

Description:

Add's sensor for Flume water sensor found here: https://flumetech.com/

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
sensor:
  # Flume
  - platform: flume
    username: YOUR_FLUME_USERNAME
    password: YOUR_FLUME_PASSWORD
    client_id: YOUR_FLUME_CLIENT_ID
    client_secret: YOUR_FLUME_CLIENT_SECRET

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@project-bot project-bot bot added this to Needs review in Dev Oct 5, 2019
Dev automation moved this from Needs review to Review in progress Oct 6, 2019
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
Add support for Flume API and sensor
Timezone is now a required option in configuration.yaml
Add flume to be ommited from testing. Add @ChrisMandich as Flume Code owner
Added Documentation URL for Flume to HASS.IO
Added missing Newline at EOF.
Update sensor to not required Device ID for configuration. Now loops through all available Type 2 devices and adds them as an entity.
Resolved errors related to code owners and requirements. Using hassfest and gen_requirements_all
Implemented recommendations from @Quentame. Including time zone from Home Assistant, Updated variable names, and Consolidated duplicate functions.
This includes: components name, using f-strings instead of concat, snake_case for variables, constants for the addition of future device types, clearer errors, and removed variables no longer in use.
Restored unit_of_measurement. Updated return to "gal".
Include protected attributes in setup_platform.
homeassistant/components/flume/sensor.py:63:11: W0703: Catching too general exception Exception (broad-except)
homeassistant/components/flume/sensor.py:133:8: R1720: Unnecessary "else" after "raise" (no-else-raise)
homeassistant/components/flume/sensor.py:162:8: R1705: Unnecessary "else" after "return" (no-else-return)
homeassistant/components/flume/sensor.py:236:8: R1720: Unnecessary "else" after "raise" (no-else-raise)
I'm okay with the broad exception clause.
homeassistant/components/flume/sensor.py:65:11: W0703: Catching too general exception Exception (broad-except)
Add more specific exceptions for Try/Except.
@ChrisMandich
Copy link
Contributor Author

@Quentame I'm fairly new to contributing to Github projects. I believe I corrected all they Pylint errors. Can you guide me on what 'codecov/patch' is and what is required to get complete this successfully?

@frenck
Copy link
Member

frenck commented Oct 15, 2019

All communication to external devices or services must be wrapped in an external Python library hosted on pypi. See the: https://developers.home-assistant.io/docs/en/development_checklist.html

Furthermore, the config flow requires tests, please add those to make codecov pass.

Lastly, please open up a PR in our documentation repository for this new integration. Please be sure to link the PR in the openings post of this PR.

Thanks! 👍

@Quentame
Copy link
Member

Hi again @ChrisMandich !

Me too, I'm quite new in contributions and also in Python dev actually. But most of the comments I've added to your PR are ones I already got while adding config flow to Linky and iCloud components.

I've haven't built a component from the ground up but your PR pushed me to review, hoping someone else will join the game 😉 (I'm not in HA team but new integration with passed pipeline attracts).

I think you should add your untested files to the .coveragerc file (be careful it is checked on the checklist but not made), if it's not that, I don't know 😬

I know that config flow needs tests but this PR doesn't have it (you should consider adding it perhaps).

@frenck : Where are the test/cov requirements written in the documentation ?

In the component checklit there is nothing about the coverage and tests are just strongly recommended.

Copy link
Member

@balloob balloob 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 rework how APIs are called. Look at the codebase and search for @Throttle.

Reccomended by @balloob as it is already a core requirement
Add proposed changes from @balloob
pytz is a core dependency, removing flume's requirement for it.
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/flume/sensor.py Outdated Show resolved Hide resolved
Added @MartinHjelmare recommended changes.
Resolving PyLint error
@michaeldavie
Copy link
Contributor

Thank for bringing this forward. I've been casually working on a similar project, and I have a couple suggestions:

  • My approach uses single data object to support multiple sensors, and uses a single API call with multiple queries to refresh them at the same time. It's similar to what many of the weather components do, and I think it makes sense for Flume as well.
  • I've worked out a few queries/sensors to mirror the data provided in the native Flume app.
  • I haven't completely worked out the OAuth2 support, and it looks like you might not have either. The tokens issued by Flume expire after a week, so we need to handle either refreshing a token or requesting a new one when the API returns an error indicating that the token is expired.

There's also a thread that a few Flume users are following that might be useful.

Let me know if you have any questions about my code, or if there's anything I can do to help.

Remove `KeyError`. Add length check for flume entity list before adding.
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.

Looks good!

@Quentame
Copy link
Member

So it can be merged ?

@MartinHjelmare MartinHjelmare merged commit 08432c7 into home-assistant:dev Nov 23, 2019
Dev automation moved this from Review in progress to Done Nov 23, 2019
@lock lock bot locked and limited conversation to collaborators Nov 24, 2019
@ChrisMandich ChrisMandich deleted the add-flume-support branch December 15, 2019 22:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants