Skip to content

[py] moved all getters in fedcm/account.py into descriptor object.#14858

Merged
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:trunk
Dec 6, 2024
Merged

[py] moved all getters in fedcm/account.py into descriptor object.#14858
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:trunk

Conversation

@sandeepsuryaprasad
Copy link
Copy Markdown
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Dec 5, 2024 •
edited by qodo-code-review Bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Moved all getters in fedcm/account.py to descriptor object

Motivation and Context

  • This eliminates to have multiple getters doing similar job.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement

Description

  • Introduced a new _AccountDescriptor class to implement the descriptor pattern for the Account class attributes.
  • Replaced multiple property methods in the Account class with instances of _AccountDescriptor, simplifying the class structure.
  • Enhanced code maintainability by centralizing attribute access logic in the _AccountDescriptor class.

Changes walkthrough 📝

Relevant files
Enhancement
account.py
Refactor Account class to use descriptor pattern                 

py/selenium/webdriver/common/fedcm/account.py

  • Introduced _AccountDescriptor class for descriptor pattern.
  • Replaced property methods with _AccountDescriptor instances.
  • Simplified Account class by using descriptors for attributes.
    		•  +21/-36 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Dec 5, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Hints
    The descriptor class should specify return type hints for init and set methods for better code clarity and type safety

    Documentation
    The _AccountDescriptor class lacks docstring explaining its purpose and usage

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Dec 5, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure proper type conversion for enum values when retrieving them through the descriptor

    The descriptor's get method should return the correct type for login_state.
    Since it's an Enum value, it should convert the string value to a LoginState enum
    instance.

    py/selenium/webdriver/common/fedcm/account.py [31-32]

    -def __get__(self, obj, cls) -> Optional[str]:
-    return obj._account_data.get(self.name)
+def __get__(self, obj, cls) -> Optional[Union[str, LoginState]]:
+    value = obj._account_data.get(self.name)
+    if self.name == "loginState" and value is not None:
+        return LoginState(value)
+    return value
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical fix that ensures type safety and correct behavior. Without this change, the login_state property would return a string instead of the expected LoginState enum value, potentially causing type-related bugs.

    		9
    General
    Improve error message clarity by including specific attribute information

    The descriptor's set error message should include the attribute name for better
    debugging.

    py/selenium/webdriver/common/fedcm/account.py [34-35]

     def __set__(self, obj, value) -> None:
-    raise AttributeError("Cannot set readonly attribute")
+    raise AttributeError(f"Cannot set readonly attribute '{self.name}'")
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While this improves error message clarity for debugging, it's a minor enhancement that doesn't affect functionality. The current error message is already sufficient for basic error reporting.

    		4

    💡 Need additional feedback ? start a PR chat

    navin772
    navin772 reviewed Dec 6, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    Member

    @navin772 navin772 left a comment

    Choose a reason for hiding this comment

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

    LGTM! Thanks for the refactoring

    @VietND96 VietND96 merged commit 313f7a5 into SeleniumHQ:trunk Dec 6, 2024
    @VietND96
    Copy link
    Copy Markdown
    Member

    VietND96 commented Dec 6, 2024

    Thank you, @sandeepsuryaprasad !

    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot Bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @navin772 navin772 navin772 left review comments

    @VietND96 VietND96 VietND96 approved these changes

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @sandeepsuryaprasad @VietND96 @navin772