Skip to content

Rename current_consumption to power #1349

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

Closed
wants to merge 2 commits into from

Conversation

ryenitcher
Copy link
Contributor

  • Note: This is a breaking change.
  • Fix: Rename current_consumption to power in energy modules, to deconflict and clarify.
  • Fix: Report 0 instead of None for current when current is zero.
  • Fix: Report 0 instead of None for voltage when voltage is zero (not that this was possible to see).

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.13%. Comparing base (cb89342) to head (1c25091).
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1349   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files         122      122           
  Lines        7867     7867           
  Branches      810      810           
=======================================
  Hits         7248     7248           
  Misses        454      454           
  Partials      165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sdb9696
Copy link
Collaborator

sdb9696 commented Dec 11, 2024

I agree with the setting to zero, but I’m not sure about the value of the rename. Whilst I get that it’s a bit confusing, we’re still left with consumption_today, consumption_this_month etc. It’s a lot of churn to deprecate all this, when updating the docstrings to make clearer this is what the device reports as power could suffice.

@ryenitcher
Copy link
Contributor Author

Yeah I was starting to doubt the cost of the change relative to its’ value. I can see that it would cost the long term history graphs of the power draw metrics in home assistant, but from what I could tell it would leave the energy monitoring integration mostly intact since that uses the accumulated consumption metrics instead of the instantaneous demand values.

I think the term just confuses me since it is current_consumption rather than consumption_current or consumption_now to match the pattern of the other metrics. I see that it is inherited from TPLink themselves though, so I suppose if you are trying to stay as close to the way TPLink phrased it, the current name makes sense (although I’m still not a fan of overloading the term current in this context).

I’ll get the zero setting extracted into a separate PR, since there’s no use in holding that back for this PR.

@rytilahti rytilahti added the breaking change Breaking change label Dec 11, 2024
@ryenitcher ryenitcher force-pushed the dev-power branch 2 times, most recently from 6c83936 to 0963d6f Compare January 25, 2025 01:34
- Fix: Report `0` instead of `None` for current when current is zero.
- Fix: Report `0` instead of `None` for voltage when voltage is zero (not that this was possible to see).
@ryenitcher
Copy link
Contributor Author

I've extracted the zero power reporting fix into PR #1483

- Note: This is a breaking change.
- Fix: Rename `current_consumption` to `power` in energy modules, to deconflict and clarify.
@rytilahti
Copy link
Member

We could still look into renaming while keeping the deprecated names available for the time being. The only one we cannot change that easily is the feature identifier (which is used as a unique key).

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.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Apr 26, 2025
@github-actions github-actions bot closed this May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants