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

Small update to home load handling #512

Merged
merged 105 commits into from
Sep 27, 2024
Merged

Conversation

tmenguy
Copy link
Contributor

@tmenguy tmenguy commented Sep 26, 2024

Installed pre-commit too

Summary by Sourcery

Enhance the home update logic by adding a flag to detect updates when person data changes, and install pre-commit to improve code quality checks.

Enhancements:

  • Add a flag to indicate when there is an update to the house based on person updates.

Chores:

  • Install pre-commit to the project.

tmenguy and others added 30 commits February 20, 2024 01:03
…, introduces a sum energy and an helper to be used in homeassistant primarily
Copy link

sourcery-ai bot commented Sep 26, 2024

Reviewer's Guide by Sourcery

This pull request makes a small update to the home load handling in the src/pyatmo/home.py file. The change adds a flag to indicate that the house has been updated when there's a person update.

Sequence Diagram

No sequence diagram generated.

File-Level Changes

Change Details Files
Added a flag to indicate house update on person status change
  • Introduced a new variable 'has_an_update' and set it to True when processing person status updates
  • This change implies that any person status update is considered a house update
src/pyatmo/home.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tmenguy - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The commit message could be more descriptive. Please provide context on why this change was made and how it improves home load handling.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -179,6 +179,8 @@ async def update(
self.rooms[room["id"]].update(room)

for person_status in data.get("persons", []):
# if there is a person update, it means the house has been updated
Copy link

Choose a reason for hiding this comment

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

suggestion (performance): Consider optimizing the has_an_update flag assignment

The current implementation sets has_an_update = True for each person update. Consider moving this assignment before the loop for efficiency. Also, review if all person updates truly imply a house update, and ensure the flag is being used effectively in the broader context.

has_an_update = bool(data.get("persons"))
for person_status in data.get("persons", []):
    if person := self.persons.get(person_status["id"]):

@cgtobi cgtobi merged commit c0c000a into jabesq-org:development Sep 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants