Skip to content

Conversation

@M1troll
Copy link
Contributor

@M1troll M1troll commented Jun 21, 2024

User description

Description

The main change is changing the return type hint for def get_downloadable_files(self) from dict to List[str], which is actually what is returned.

Also some cosmetic improvements have been added.

Motivation and Context

I caught some error from my static type checker () and saw that get_downloadable_files has the wrong type annotation and decided to fix it =)
image

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, Enhancement


Description

  • Changed the return type hint for get_downloadable_files from dict to List[str] to accurately reflect the returned value.
  • Improved the formatting of import statements by grouping and ordering them.
  • Updated type hints for methods __exit__, get_cookie, and others to use Optional and Type from typing.
  • Applied minor cosmetic improvements and formatting changes across the file, including docstring adjustments and line spacing.

Changes walkthrough 📝

Relevant files
Enhancement
webdriver.py
Fix type hints and improve code formatting in `webdriver.py`

py/selenium/webdriver/remote/webdriver.py

  • Changed the return type hint for get_downloadable_files from dict to
    List[str].
  • Improved import statements formatting.
  • Updated type hints for several methods.
  • Applied minor cosmetic improvements and formatting changes.
  • +23/-31 

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

    @qodo-merge-pro qodo-merge-pro bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue Review effort [1-5]: 3 labels Jun 21, 2024
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The method get_downloadable_files was changed from returning a dict to List[str]. Ensure that the actual implementation of this method and its usage across the project align with this change to avoid runtime errors.
    Code Consistency:
    The PR includes various cosmetic changes such as reformatting import statements and adjusting docstrings. While these changes improve readability, ensure they adhere to the project's coding standards.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 21, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for the script parameter to ensure it is a non-empty string

    The pin_script method should validate the script parameter to ensure it is a non-empty
    string before proceeding.

    py/selenium/webdriver/remote/webdriver.py [375]

    +if not isinstance(script, str) or not script.strip():
    +    raise ValueError("Script must be a non-empty string")
     script_key_instance = ScriptKey(script_key)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This is a valid suggestion to ensure robustness of the pin_script method by validating input, although it's not a critical bug fix.

    7
    Add validation for the name parameter to ensure it is a non-empty string

    The get_cookie method should validate the name parameter to ensure it is a non-empty
    string before proceeding.

    py/selenium/webdriver/remote/webdriver.py [586]

    +if not isinstance(name, str) or not name.strip():
    +    raise ValueError("Cookie name must be a non-empty string")
     """Get a single cookie by name. Returns the cookie if found, None if
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion correctly identifies a potential improvement in input validation for the get_cookie method, enhancing the method's robustness.

    7

    def get_downloadable_files(self) -> List[str]:
    """Retrieves the downloadable files as a map of file names and their
    corresponding URLs."""
    corresponding URLs.
    Copy link
    Member

    Choose a reason for hiding this comment

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

    We need to update the docstring here

    @VietND96 VietND96 changed the title Fix typing in remote webdriver [py] Fix typing in remote webdriver Nov 12, 2024
    @VietND96 VietND96 added the C-py Python Bindings label Nov 19, 2024
    @cgoldberg
    Copy link
    Member

    Is this still relevant? If so, can you resolve the merge conflicts?

    @M1troll M1troll force-pushed the fix-typing-in-remote-webdriver branch from fc3f165 to d474e2f Compare May 14, 2025 01:16
    @M1troll M1troll closed this May 14, 2025
    @M1troll M1troll force-pushed the fix-typing-in-remote-webdriver branch from d474e2f to ff55efd Compare May 14, 2025 01:29
    @cgoldberg
    Copy link
    Member

    If there was anything useful in that branch, please submit another PR.

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

    Labels

    C-py Python Bindings P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants