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

fix type errors for service.py, cdp.py, webelement.py and remote_connection.py #14448

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Aug 28, 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

Fixes the mypy type errors for multiple files.

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

Bug fix


Description

  • Fixed type errors across multiple files by adding type annotations and using cast for type hinting.
  • Added checks for None values and appropriate error handling in service-related files.
  • Updated timeout initialization in remote_connection.py to use getdefaulttimeout.
  • Ensured send_keys method in webelement.py handles values as tuples.
  • Added checks for executable paths and removed unnecessary type ignore comments.

Changes walkthrough 📝

Relevant files
Bug fix
cdp.py
Add type annotations for event channels                                   

py/selenium/webdriver/common/bidi/cdp.py

  • Added type annotations for sender and receiver variables.
+2/-0     
service.py
Fix type errors and add type casting                                         

py/selenium/webdriver/common/service.py

  • Used cast for type hinting on log_output.
  • Added check for None path in start method.
  • Used cast for stdout and stderr in subprocess.
  • Added type ignores for Windows-specific startup info.
  • +14/-9   
    remote_connection.py
    Update timeout initialization for remote connection           

    py/selenium/webdriver/remote/remote_connection.py

    • Updated timeout initialization with getdefaulttimeout.
    +3/-3     
    webelement.py
    Fix type error in send_keys method                                             

    py/selenium/webdriver/remote/webelement.py

    • Changed value to a tuple for send_keys method.
    +1/-1     
    service.py
    Add type annotations and remove type ignore                           

    py/selenium/webdriver/webkitgtk/service.py

  • Added type annotation for DEFAULT_EXECUTABLE_PATH.
  • Removed type ignore comment.
  • +3/-3     
    service.py
    Add executable check and remove type ignore                           

    py/selenium/webdriver/wpewebkit/service.py

  • Added check and exception for missing executable.
  • Removed type ignore comment.
  • +3/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Bug
    The self._path is checked for None after it's already used, which could lead to a TypeError if it's None.

    Possible Runtime Error
    Raising a ValueError if WPEWebDriver is not found in PATH might cause issues on systems where it's not installed.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Aug 28, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the tuple creation to maintain the expected input format

    The value assignment should be a tuple of a single string, not a tuple containing
    the result of "\n".join(remote_files). This ensures that value remains a tuple of
    strings as expected by the send_keys method.

    py/selenium/webdriver/remote/webelement.py [229]

    -value = tuple("\n".join(remote_files))
    +value = ("\n".join(remote_files),)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion corrects a potential bug by ensuring that 'value' remains a tuple of strings, which is crucial for the 'send_keys' method to function correctly.

    9
    Enhancement
    Use a more specific exception type for better error handling and clarity

    Consider using a more specific exception type instead of the general
    WebDriverException. For example, you could create a custom exception like
    ServicePathException for this specific case.

    py/selenium/webdriver/common/service.py [99-100]

     if self._path is None:
    -    raise WebDriverException("Service path cannot be None.")
    +    raise ServicePathException("Service path cannot be None.")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific exception type can improve error handling and make the code more understandable. However, creating a new exception class might not be necessary unless there are multiple places where this specific error needs to be handled differently.

    7
    ✅ Defer the executable existence check to improve flexibility in class usage
    Suggestion Impact:The commit removed the immediate check for the WPEWebDriver executable, aligning with the suggestion to defer this check.

    code diff:

     DEFAULT_EXECUTABLE_PATH = shutil.which("WPEWebDriver")
    -if DEFAULT_EXECUTABLE_PATH is None:
    -    raise ValueError("WPEWebDriver executable not found in PATH")

    Instead of raising a ValueError immediately if the WPEWebDriver executable is not
    found, consider deferring this check to when the Service is actually used. This
    allows for more flexibility in how the Service class is instantiated and used.

    py/selenium/webdriver/wpewebkit/service.py [22-24]

     DEFAULT_EXECUTABLE_PATH = shutil.which("WPEWebDriver")
    -if DEFAULT_EXECUTABLE_PATH is None:
    -    raise ValueError("WPEWebDriver executable not found in PATH")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Deferring the check allows for more flexible instantiation and usage of the Service class, but it could also delay error detection, which might not be ideal in all scenarios.

    6

    @pujagani pujagani added the C-py label Sep 2, 2024
    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants