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

Flexpool.io Sensors Integration #47995

Closed
wants to merge 32 commits into from

Conversation

DatDraggy
Copy link

@DatDraggy DatDraggy commented Mar 16, 2021

Proposed change

I am adding a few sensors for the Ethereum mining pool Flexpool.io to homeassistant as my first integration.
I've been asked by other HA users to try my luck at an integration for this pool.
As mentioned, this is the first time working on HA for me. I've looked at other sensor integrations to get a grasp of how that could look like, but I'm not sure if what I've done makes sense or could be done better.

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 Configuration:

Example address for testing 0x5F8A1ae37f7caDA442241BE98b507E2455A3121D

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:

@DatDraggy DatDraggy changed the title Flexpool integration Flexpool.io Sensors Integration Mar 16, 2021
homeassistant/components/flexpool/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/strings.json Outdated Show resolved Hide resolved
homeassistant/components/flexpool/strings.json Outdated Show resolved Hide resolved
@DatDraggy
Copy link
Author

Checks are done. Thank you for your help with the test, Nathan. :)

@DatDraggy
Copy link
Author

DatDraggy commented Mar 30, 2021

I believe review approval is still missing or do you not have permission to fully review PRs? I don't quite know how things work here.

@ntilley905
Copy link
Contributor

If you're asking me, that is correct, I am not a core developer, only a contributor. My review does not give you reviewer approval. One of those devs will be around at some point, they're pretty busy so be patient!

@DatDraggy
Copy link
Author

Okay, I thought so. Thank you so much for taking your time to review it anyway. There were changes needed after all

@frenck frenck self-requested a review May 1, 2021 10:19
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Hi there @DatDraggy 👋

I did an initial review pass, to get some things going and moving.
I think the biggest change you should consider, is implementing the DataUpdateCoordinator.

Some of the suggestions in this review, might require you to rebase this PR onto the latest dev branch first.

../Frenck 🚂

homeassistant/components/flexpool/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/config_flow.py Outdated Show resolved Hide resolved
tests/components/flexpool/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/flexpool/test_config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/sensors.py Outdated Show resolved Hide resolved
homeassistant/components/flexpool/strings.json Outdated Show resolved Hide resolved
homeassistant/components/flexpool/sensors.py Outdated Show resolved Hide resolved
DatDraggy and others added 17 commits May 5, 2021 14:00
Co-authored-by: Nathan Tilley <nathan@tilley.xyz>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
@DatDraggy
Copy link
Author

Alright, all feedback was implemented as far as possible. The official python package kind of limits the usage of the update coordinator, but it does reduce the amount of api calls.
I've tested all and it works. For some reason the workflow tests don't run anymore so I'm not sure if they pass.

@DatDraggy
Copy link
Author

There will be a new api wrapper soon so I'm going to put this PR on hold for now until I have implemented the new one. It improve performance drastically.

@frenck frenck removed their request for review May 25, 2021 17:45
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Look at it again, added a comment (besides the comments that are still open at this point). Could you take a look at those @DatDraggy? 👍

Comment on lines +1215 to +1218
homeassistant/components/flexpool/__init__.py
homeassistant/components/flexpool/const.py
homeassistant/components/flexpool/helper.py
homeassistant/components/flexpool/sensor.py
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this file alphabetically sorted.

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 10, 2021
@github-actions github-actions bot closed this Sep 17, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants