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 pointer_input.py, wheel_input.py and firefox/options.py #14476

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Sep 6, 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 to resolve mypy issues.
  • Added type hints and updated method signatures in firefox/options.py.
  • Updated create_pause methods in pointer_input.py and wheel_input.py to accept both integers and floats.
  • Improved type handling for log_output in chromium/service.py.

Changes walkthrough 📝

Relevant files
Bug fix
service.py
Fix type handling for log_output in Chromium service         

py/selenium/webdriver/chromium/service.py

  • Added type hint for log_output as Optional[IOBase].
  • Handled IOBase type for log_output.
  • +4/-1     
    pointer_input.py
    Update create_pause method to accept int or float               

    py/selenium/webdriver/common/actions/pointer_input.py

    • Changed create_pause method to accept Union[int, float].
    +2/-1     
    wheel_input.py
    Update create_pause method to accept int or float               

    py/selenium/webdriver/common/actions/wheel_input.py

    • Changed create_pause method to accept Union[int, float].
    +1/-1     
    options.py
    Add type hints and update method signatures in Firefox options

    py/selenium/webdriver/firefox/options.py

  • Added type hints for various attributes and methods.
  • Updated enable_mobile method to use Optional[str].
  • +10/-5   
    service.py
    Remove type ignore comment in WPEWebKit service                   

    py/selenium/webdriver/wpewebkit/service.py

    • Removed # type: ignore comment.
    +1/-1     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Type Inconsistency
    The create_pause method now accepts both int and float, but the type hint for pause_duration is not updated in the method signature.

    Missing Import
    The Union type is used in the create_pause method signature, but it's not imported from the typing module.

    Potential Bug
    The enable_mobile method's default value for android_package has changed from a string to an Optional[str], which might affect existing code relying on the previous behavior.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use a more specific type hint for a parameter to improve type checking and code clarity

    Consider using a more specific type hint for log_output in the method signature.
    Instead of SubprocessStdAlias, use Union[str, IOBase, None] to accurately represent
    the possible types.

    py/selenium/webdriver/chromium/service.py [35-43]

     def __init__(
         self,
         executable_path: str = None,
         port: int = 0,
         service_args: typing.Optional[typing.List[str]] = None,
    -    log_output: SubprocessStdAlias = None,
    +    log_output: Union[str, IOBase, None] = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         **kwargs,
     ) -> None:
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to use Union[str, IOBase, None] for log_output is accurate and improves type clarity, reflecting the actual types handled in the code.

    9
    Add specific type hints for method parameters to improve type checking and code clarity

    Consider using a more specific type hint for android_activity and device_serial
    parameters in the enable_mobile method. This will improve type checking and code
    clarity.

    py/selenium/webdriver/firefox/options.py [102-105]

     def enable_mobile(
    -    self, android_package: Optional[str] = "org.mozilla.firefox", android_activity=None, device_serial=None
    +    self, android_package: Optional[str] = "org.mozilla.firefox", android_activity: Optional[str] = None, device_serial: Optional[str] = None
     ):
         super().enable_mobile(android_package, android_activity, device_serial)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding Optional[str] type hints for android_activity and device_serial enhances type checking and clarity, aligning with the method's intended use.

    9
    Simplify a parameter type hint while maintaining the same functionality

    Consider using float instead of Union[int, float] for pause_duration parameter, as
    the method already converts it to milliseconds using int(). This simplifies the type
    hint while maintaining the same functionality.

    py/selenium/webdriver/common/actions/pointer_input.py [64-65]

    -def create_pause(self, pause_duration: Union[int, float] = 0) -> None:
    +def create_pause(self, pause_duration: float = 0) -> None:
         self.add_action({"type": "pause", "duration": int(pause_duration * 1000)})
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion to use float instead of Union[int, float] is valid as the method converts the value to an integer, simplifying the type hint without changing functionality.

    8
    Initialize a dictionary with type annotations at the beginning of a method to improve code structure and type checking

    Consider initializing the opts dictionary with type annotations at the beginning of
    the to_capabilities method to improve code readability and type checking.

    py/selenium/webdriver/firefox/options.py [107-114]

     def to_capabilities(self) -> dict:
         """Marshals the Firefox options to a `moz:firefoxOptions` object."""
         # This intentionally looks at the internal properties
         # so if a binary or profile has _not_ been set,
         # it will defer to geckodriver to find the system Firefox
         # and generate a fresh profile.
         caps = self._caps
         opts: Dict[str, Any] = {}
    +    caps["moz:firefoxOptions"] = opts
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: While the suggestion to initialize opts with type annotations is correct and improves readability, it is a minor enhancement as the existing code already functions correctly.

    7

    Copy link

    codecov bot commented Sep 6, 2024

    Codecov Report

    Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

    Project coverage is 57.64%. Comparing base (8fc4299) to head (a2b8f37).
    Report is 7 commits behind head on trunk.

    Files with missing lines Patch % Lines
    py/selenium/webdriver/firefox/options.py 77.77% 1 Missing and 1 partial ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #14476      +/-   ##
    ==========================================
    - Coverage   57.65%   57.64%   -0.02%     
    ==========================================
      Files          89       89              
      Lines        5571     5586      +15     
      Branches      236      245       +9     
    ==========================================
    + Hits         3212     3220       +8     
    + Misses       2123     2121       -2     
    - Partials      236      245       +9     

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

    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.

    Thank you @navin772 !

    @harsha509 harsha509 merged commit 05bce9b into SeleniumHQ:trunk Sep 10, 2024
    13 checks passed
    @navin772 navin772 deleted the fix-mypy-errors-5 branch September 10, 2024 16:45
    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.

    2 participants