Skip to content

[py] Refactored server.py in a more pythonic approach. #15840

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

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

Conversation

sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jun 1, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Refactored server.py in a more pythonic approach by implementing property descriptors.

🔧 Implementation Notes

Implemented standard getters and setters to set various server parameters.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Refactored server.py to use Python property descriptors

  • Replaced validation methods with property setters

  • Improved attribute validation and encapsulation

  • Enhanced code readability and maintainability


Changes walkthrough 📝

Relevant files
Enhancement
server.py
Refactor server attributes using properties and setters   

py/selenium/webdriver/remote/server.py

  • Replaced explicit validation methods with property setters/getters
  • Added property decorators for path, port, version, log_level, and env
  • Moved validation logic into property setters for encapsulation
  • Updated status_url to be a property
  • +42/-18 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-py Python Bindings label Jun 1, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 1, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()
    • Ensure JavaScript in href attributes works properly when clicked in Firefox 42.0

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver
    • Address issue where subsequent ChromeDriver instantiations fail after the first one succeeds

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

    Class Reference

    The __class__.__name__ reference in error messages is used without proper context. This should be replaced with the actual class name or self.class.name to ensure correct error messages.

        raise TypeError(f"{__class__.__name__}.__init__() got an invalid port: '{port}'")
    if not (0 <= port <= 65535):
    Missing Docstrings

    The newly added properties lack docstrings explaining their purpose and usage, which reduces code maintainability and makes it harder for other developers to understand the API.

    @property
    def status_url(self):
        host = self.host if self.host is not None else "localhost"
        return f"http://{host}:{self.port}/status"
    
    @property
    def path(self):
        return self._path
    
    @path.setter
    def path(self, path):
        if path and not os.path.exists(path):
            raise OSError(f"Can't find server .jar located at {path}")
        self._path = path
    
    @property
    def port(self):
        return self._port
    
    @port.setter
    def port(self, port):
        try:
            port = int(port)
        except ValueError:
            raise TypeError(f"{__class__.__name__}.__init__() got an invalid port: '{port}'")
        if not (0 <= port <= 65535):
            raise ValueError("port must be 0-65535")
        self._port = port
    
    @property
    def version(self):
        return self._version
    
    @version.setter
    def version(self, version):
        if version:
            if not re.match(r"^\d+\.\d+\.\d+$", str(version)):
                raise TypeError(f"{__class__.__name__}.__init__() got an invalid version: '{version}'")
        self._version = version
    
    @property
    def log_level(self):
        return self._log_level
    
    @log_level.setter
    def log_level(self, log_level):
        levels = ("SEVERE", "WARNING", "INFO", "CONFIG", "FINE", "FINER", "FINEST")
        if log_level not in levels:
            raise TypeError(f"log_level must be one of: {', '.join(levels)}")
        self._log_level = log_level
    
    @property
    def env(self):
        return self._env
    
    @env.setter
    def env(self, env):
        if env is not None and not isinstance(env, collections.abc.Mapping):
            raise TypeError("env must be a mapping of environment variables")
        self._env = env

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 1, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    sandeepsuryaprasad commented Jun 3, 2025

    @cgoldberg can you please trigger CI for this one

    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