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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a base class for Intellifire entities #65077
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few comments and suggestions to help you get started.
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
cf538bc
to
c35f40d
Compare
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Testing the new changes locally now.. They work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few extra tweaks.
And since you are reordering things already in sensor.py I would reorder them instead to have this order:
- dataclasses (
IntellifireSensorRequiredKeysMixin
/IntellifireSensorEntityDescription
) - descriptions (
INTELLIFIRE_SENSORS
) - private functions (
_time_remaining_to_timestamp
) - main setup (
async_setup_entry
) - entity classes (
IntellifireSensor
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to tick "request changes" in the review - please look above for the requested changes.
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
So I tried to do this BUT!!!!: When I have the And then when I try to run this code I get the following: As I understand (unlike say C) there is no way to do forward declarations in python ... so I'm sure you can suggest a cool "clean" pythonic way of obtaining this order :) |
Everything should be ordered accordingly.... :) |
@epenet / @frenck -> I want to make an additional PR based on this codebase. It's not yet merged into
I'm assuming that the best approach is to wait until this code is merged into |
Breaking change
Proposed change
@epenet suggested I make a shared common
entity.py
class. This is a PR attempting to do this :) I'm not sure I 100% did this the cleanest way possible but hopefully we can fix that during the review process.Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: