Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[py] Refactored remote/client_config.py by moving properties into descriptor object #14899

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Dec 14, 2024

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

Refactored remote/client_config.py by moving whole bunch of getters and setters to descriptor object

Motivation and Context

Refactored remote/client_config.py by moving whole bunch of getters and setters to descriptor object

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 _ClientConfigDescriptor class to handle property access in a more maintainable way
  • Replaced 13 pairs of getter/setter methods with descriptor-based properties, significantly reducing code duplication
  • Maintained all existing functionality while making the code more concise and easier to maintain
  • Properties affected include: remote_server_addr, keep_alive, proxy, ignore_certificates, timeout, ca_certs, username, password, auth_type, token, user_agent, and extra_headers

Changes walkthrough 📝

Relevant files
Enhancement
client_config.py
Refactor properties using descriptor pattern                         

py/selenium/webdriver/remote/client_config.py

  • Added new _ClientConfigDescriptor class to handle property descriptors
  • Replaced multiple getter/setter methods with descriptor-based
    properties
  • Simplified code by removing redundant property decorators and their
    implementations
  • Maintained same functionality while reducing code duplication
  • +38/-170

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

    Copy link
    Contributor

    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

    Documentation
    The docstring for init_args_for_pool_manager property seems incorrect - it has the same description as ignore_certificates

    Code Design
    The _ClientConfigDescriptor class could validate input types in set method to maintain type safety, since the original properties had type hints

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Enforce type safety for properties that require specific types to prevent runtime errors

    Add type validation in the descriptor for properties with specific types (like
    auth_type which should be AuthType enum) to prevent runtime errors from invalid
    assignments.

    py/selenium/webdriver/remote/client_config.py [66-67]

    -auth_type = _ClientConfigDescriptor("_auth_type")
    -"""Gets and Sets the type of authentication to the remote server."""
    +class TypedDescriptor(_ClientConfigDescriptor):
    +    def __init__(self, name, expected_type):
    +        super().__init__(name)
    +        self.expected_type = expected_type
    +    
    +    def __set__(self, obj, value):
    +        if not isinstance(value, self.expected_type):
    +            raise TypeError(f"{self.name} must be of type {self.expected_type.__name__}")
    +        super().__set__(obj, value)
     
    +auth_type = TypedDescriptor("_auth_type", AuthType)
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion introduces strong type validation for properties like auth_type, which is critical for preventing runtime errors and ensuring type safety. This is particularly important for enum-based properties.

    9
    Add validation in property setters to prevent invalid values for critical configuration parameters

    Add validation in the descriptor's set method to ensure that required properties
    like remote_server_addr cannot be set to None or empty string, as these are critical
    for the client configuration.

    py/selenium/webdriver/remote/client_config.py [43-44]

     def __set__(self, obj, value) -> None:
    +    if self.name == '_remote_server_addr' and not value:
    +        raise ValueError("remote_server_addr cannot be empty")
         obj.__dict__[self.name] = value
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial validation for required configuration parameters, preventing potential runtime errors by ensuring critical values like remote_server_addr are not empty or None. This is important for maintaining system stability.

    8
    Handle undefined attributes gracefully by returning None instead of raising AttributeError

    Initialize instance attributes in init to avoid AttributeError when accessing
    properties before they are set. Currently, direct attribute access could fail if a
    property is accessed before being set.

    py/selenium/webdriver/remote/client_config.py [37-39]

     class _ClientConfigDescriptor:
         def __init__(self, name):
             self.name = name
    +    
    +    def __get__(self, obj, cls):
    +        return obj.__dict__.get(self.name)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves robustness by gracefully handling uninitialized properties, preventing AttributeError exceptions that could occur when accessing properties before they are set.

    7

    @sandeepsuryaprasad sandeepsuryaprasad changed the title [py] Refactored remote/client_config.py by moving properties into descriptor object [py] Refactored remote/client_config.py by moving properties into descriptor object Dec 15, 2024
    # for free to join this conversation on GitHub. Already have an account? # to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant