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

feat: segregate connect/auth/refresh/enable device duties #233

Merged
merged 8 commits into from
Jul 24, 2024

Conversation

chemelli74
Copy link
Member

@chemelli74 chemelli74 commented Jul 19, 2024

Summary by CodeRabbit

  • New Features

    • Enhanced device connection reliability with a retry mechanism for socket connections.
    • Introduced a new method for streamlined authentication and status refresh.
    • Improved logging for device state changes and authentication processes.
    • Added a new exception class for better error handling related to connection failures.
  • Bug Fixes

    • Addressed potential transient connection failures through retry logic.
  • Tests

    • Refactored tests for improved clarity and maintainability using parameterization.

@chemelli74 chemelli74 added the breaking change A change that is not backwards compatible label Jul 19, 2024
Copy link
Contributor

coderabbitai bot commented Jul 19, 2024

Walkthrough

The recent changes enhance the connection and authentication processes in the device management code, improving reliability and logging. A new method for refreshing authentication capabilities was introduced, and existing methods were adjusted to streamline operations and improve traceability. In testing, the transition to a synchronous approach simplifies test structure, enhancing clarity and maintainability. Overall, these updates contribute to a more robust and user-friendly device experience.

Changes

File Change Summary
midealocal/device.py Improved connection logic with retry mechanism; added a new authentication method and enhanced logging for state changes.
midealocal/exceptions.py Introduced a new exception class CannotConnect for better connection error handling.
tests/device_test.py Restructured tests by removing asynchronous inheritance and introduced parameterization for simplified testing of connection scenarios.

Poem

🐰 In the code where devices dwell,
A tweak in flow, all is well!
Retry the connection, oh so bright,
With logs that sparkle, a clear sight.
Synchronous tests now dance with glee,
A joyful leap for you and me! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@chemelli74 chemelli74 marked this pull request as ready for review July 22, 2024 14:26
@chemelli74 chemelli74 requested a review from rokam July 22, 2024 14:27
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 11.62791% with 38 lines in your changes missing coverage. Please review.

Project coverage is 34.05%. Comparing base (c636eee) to head (c39065c).

Files Patch % Lines
midealocal/device.py 9.52% 38 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
- Coverage   36.62%   34.05%   -2.57%     
==========================================
  Files          81       81              
  Lines        7138     7142       +4     
==========================================
- Hits         2614     2432     -182     
- Misses       4524     4710     +186     

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between efa78af and 335c5db.

Files selected for processing (2)
  • midealocal/device.py (4 hunks)
  • tests/device_test.py (3 hunks)
Additional comments not posted (8)
tests/device_test.py (4)

30-30: Class restructuring: Removed inheritance from IsolatedAsyncioTestCase.

The class no longer inherits from IsolatedAsyncioTestCase, indicating a shift to synchronous testing.


Line range hint 34-49:
LGTM! Ensure consistency in device initialization.

The _setup_device fixture correctly initializes the MideaDevice instance with appropriate parameters.


61-75: Improved test readability and maintainability with pytest.mark.parametrize.

The use of pytest.mark.parametrize streamlines the testing of various scenarios for the connect method, enhancing readability and maintainability.


81-82: LGTM! Ensure proper handling of generic exceptions.

The test_connect_generic_exception method correctly tests the connect method with a generic exception.

midealocal/device.py (4)

143-143: Initialization: Set _available to False.

The _available attribute is now initialized to False, indicating the device is unavailable upon instantiation.


193-199: New method: _authenticate_refresh_enable.

The _authenticate_refresh_enable method streamlines the connection process, including authentication and status refresh. Ensure that it correctly integrates these processes.


201-236: Enhanced connection reliability with retry mechanism.

The connect method now includes a retry mechanism, allowing up to three attempts to establish a socket connection. This enhances the robustness of the connection logic.


466-469: Improved logging in enable_device method.

The enable_device method now logs when the device is enabled or disabled, providing better visibility into the device's state changes.

rokam
rokam previously requested changes Jul 22, 2024
Copy link
Contributor

@rokam rokam left a comment

Choose a reason for hiding this comment

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

I saw just a possible issue.

midealocal/device.py Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 335c5db and ca1d0ea.

Files selected for processing (1)
  • midealocal/device.py (4 hunks)
Additional comments not posted (3)
midealocal/device.py (3)

143-143: Initialization of _available attribute.

The _available attribute is now initialized to False, ensuring the device is marked as unavailable upon initialization. This change is straightforward and improves clarity.


246-246: Added debug log message in authenticate.

The debug log message for the authentication handshake enhances traceability and debugging.


469-469: Added log message in enable_device.

The log message indicating when the device is enabled or disabled improves visibility into the device's state changes.

midealocal/device.py Outdated Show resolved Hide resolved
midealocal/device.py Outdated Show resolved Hide resolved
midealocal/device.py Outdated Show resolved Hide resolved
chemelli74 and others added 3 commits July 23, 2024 17:46
<!--pre-commit.ci start-->
updates:
- astral-sh/ruff-pre-commit@v0.5.2...v0.5.4
-
commitizen-tools/commitizen@v3.27.0...v3.28.0
<!--pre-commit.ci end-->

Co-authored-by: Lucas Mindêllo de Andrade <[email protected]>
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **New Features**
- Introduced a new framework for parsing message bodies, allowing for
flexible and type-safe handling of various data types.
- Added specific parsers for boolean, integer, and enumeration types to
the message parsing logic.
- Enhanced the `MessageBody` class with a method to parse multiple
attributes automatically.

- **Tests**
- Added a comprehensive suite of unit tests for the message parsing
features, ensuring functionality and error handling across various
parser classes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Simone Chemelli <[email protected]>
@chemelli74 chemelli74 force-pushed the chemelli74-duty-segregation branch from ca1d0ea to 119879d Compare July 23, 2024 17:46
@chemelli74 chemelli74 requested a review from rokam July 23, 2024 17:47
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ca1d0ea and c39065c.

Files selected for processing (2)
  • midealocal/device.py (5 hunks)
  • midealocal/exceptions.py (1 hunks)
Additional comments not posted (5)
midealocal/exceptions.py (1)

14-16: LGTM!

The addition of the CannotConnect exception class enhances error handling by providing a specific error type for connection failures.

midealocal/device.py (4)

193-198: LGTM!

The addition of the _authenticate_refresh_capabilities method consolidates related operations, improving code organization and maintainability.


199-234: LGTM!

The modifications to the connect method, including the retry mechanism and enhanced logging, improve the robustness and traceability of the connection logic.


513-520: LGTM!

The updates to the run method streamline the connection and authentication process, improving reliability and maintainability.


464-467: LGTM!

The added logging in the enable_device method provides better visibility into the device's state changes.

Copy link
Contributor

@rokam rokam left a comment

Choose a reason for hiding this comment

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

LGTM

@chemelli74 chemelli74 merged commit 681bd79 into midea-lan:main Jul 24, 2024
9 checks passed
@rokam rokam mentioned this pull request Jul 24, 2024
rokam added a commit that referenced this pull request Jul 24, 2024
rokam added a commit that referenced this pull request Jul 24, 2024
🤖 I have created a release *beep* *boop*
---


## [2.4.0](v2.3.0...v2.4.0)
(2024-07-24)


### Features

* **cli:** set attribute from device
([#241](#241))
([6f0a109](6f0a109))
* segregate connect/auth/refresh/enable device duties
([#233](#233))
([681bd79](681bd79))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Introduced the ability to set device attributes directly from the
command-line interface.
  
- **Improvements**
- Enhanced modularity of device management functionalities for better
maintainability and clarity, addressing user feedback from issue #233.
  
- **Version Update**
- Updated the project version to 2.4.0, indicating the inclusion of new
features and improvements.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
)
self._socket.connect((self._ip_address, self._port))
_LOGGER.debug("[%s] Connected", self._device_id)
connected = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add break after connected?

@chemelli74 chemelli74 deleted the chemelli74-duty-segregation branch July 26, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A change that is not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants