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

[py] Typing package import enhancement #14283

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

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 19, 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

In this pull request, I have standardized the imports from the typing package to only include the necessary types for type hints.

Motivation and Context

I believe this approach makes the imports more decomposed, readable, and correct. From the changes, you can see that in some files, the entire package was imported as well as specific types.
Screenshot 2024-07-20 at 02 48 02

I consider this coding style inconsistent.

Initially, I started adding type hints and discovered such imports. I will add type hints in one of the following pull requests.

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

  • Standardized imports from the typing package across multiple files to improve readability and consistency.
  • Replaced general typing imports with specific type imports such as Optional, List, Dict, Union, etc.
  • Updated type hints throughout the codebase to use these specific imports.
  • Improved code consistency by ensuring all files follow the same import structure for type hints.

Changes walkthrough 📝

Relevant files
Enhancement
21 files
generate.py
Refactor typing imports for generate.py                                   

py/generate.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use direct imports like Optional, List, etc.
  • Used cast from typing directly.
  • +26/-26 
    types.py
    Refactor typing imports for types.py                                         

    py/selenium/types.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use direct imports like Union, Iterable, etc.
  • +8/-4     
    service.py
    Refactor typing imports for chrome service                             

    py/selenium/webdriver/chrome/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +7/-3     
    service.py
    Refactor typing imports for chromium service                         

    py/selenium/webdriver/chromium/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +7/-5     
    pointer_input.py
    Refactor typing imports for pointer input                               

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

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, Dict, and Any.
  • +6/-3     
    cdp.py
    Refactor typing imports for bidi cdp                                         

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

  • Replaced typing imports with specific type imports.
  • Updated type hints to use AsyncGenerator, Generator, and TypeVar.
  • +12/-7   
    script.py
    Refactor typing imports for bidi script                                   

    py/selenium/webdriver/common/bidi/script.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use List.
  • +2/-2     
    options.py
    Refactor typing imports for common options                             

    py/selenium/webdriver/common/options.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional.
  • +5/-4     
    service.py
    Refactor typing imports for common service                             

    py/selenium/webdriver/common/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, Mapping, and Union.
  • +11/-7   
    virtual_authenticator.py
    Refactor typing imports for virtual authenticator               

    py/selenium/webdriver/common/virtual_authenticator.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, Dict, and Union.
  • +10/-7   
    service.py
    Refactor typing imports for edge service                                 

    py/selenium/webdriver/edge/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +6/-3     
    service.py
    Refactor typing imports for firefox service                           

    py/selenium/webdriver/firefox/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +4/-3     
    service.py
    Refactor typing imports for IE service                                     

    py/selenium/webdriver/ie/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional and List.
  • +4/-4     
    webdriver.py
    Refactor typing imports for remote webdriver                         

    py/selenium/webdriver/remote/webdriver.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, Dict, and Type.
  • +5/-5     
    options.py
    Refactor typing imports for safari options                             

    py/selenium/webdriver/safari/options.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Dict.
  • +2/-2     
    service.py
    Refactor typing imports for safari service                             

    py/selenium/webdriver/safari/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +7/-4     
    event_firing_webdriver.py
    Refactor typing imports for event firing webdriver             

    py/selenium/webdriver/support/event_firing_webdriver.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use List, Tuple, and Any.
  • +7/-6     
    wait.py
    Refactor typing imports for support wait                                 

    py/selenium/webdriver/support/wait.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, Tuple, and Type.
  • +5/-3     
    service.py
    Refactor typing imports for webkitgtk service                       

    py/selenium/webdriver/webkitgtk/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +8/-6     
    options.py
    Refactor typing imports for wpewebkit options                       

    py/selenium/webdriver/wpewebkit/options.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Dict.
  • +3/-2     
    service.py
    Refactor typing imports for wpewebkit service                       

    py/selenium/webdriver/wpewebkit/service.py

  • Replaced typing imports with specific type imports.
  • Updated type hints to use Optional, List, and Mapping.
  • +7/-5     

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

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Jul 19, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 597f8f9)

    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

    Type Consistency
    Ensure that the changes to type hints are consistent throughout the file, especially for the log_output parameter in the __init__ method.

    Potential Functionality Change
    Verify that the changes to the get_cookie method's return type annotation don't affect its functionality or usage elsewhere in the codebase.

    Import Order
    Check if the new import order affects any existing functionality or introduces any circular dependencies.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Jul 19, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 597f8f9
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a tuple instead of a list for immutable sequences

    Consider using a tuple instead of a list for the enum attribute in the CdpProperty
    class, as it appears to be a fixed set of values that won't be modified.

    py/generate.py [212]

    -enum: List[str]
    +enum: Tuple[str, ...]
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a tuple instead of a list for the enum attribute in the CdpProperty class is a good suggestion as it indicates immutability, which can improve code clarity and potentially performance. This change aligns with best practices for handling fixed sets of values.

    7
    Enhancement
    Use keyword-only arguments to improve function call clarity and prevent errors

    Consider using keyword-only arguments for the enable_mobile method to improve
    clarity and prevent potential errors from positional argument misuse.

    py/selenium/webdriver/common/options.py [459-463]

     def enable_mobile(
         self,
    +    *,
         android_package: Optional[str] = None,
         android_activity: Optional[str] = None,
         device_serial: Optional[str] = None,
     ) -> None:
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Introducing keyword-only arguments in the enable_mobile method enhances code clarity and reduces the risk of errors from positional argument misuse. This change is beneficial for maintaining clear and robust code.

    6
    Utilize TypeAlias for clearer type alias definitions and improved static type checking support

    Consider using TypeAlias from typing to define type aliases. This can make the
    intentions clearer and provide better support for static type checkers.

    py/selenium/types.py [25-29]

    -AnyKey = Union[str, int, float]
    -WaitExcTypes = Iterable[Type[Exception]]
    -SubprocessStdAlias = Union[int, str, IO[Any]]
    +from typing import TypeAlias
     
    +AnyKey: TypeAlias = Union[str, int, float]
    +WaitExcTypes: TypeAlias = Iterable[Type[Exception]]
    +SubprocessStdAlias: TypeAlias = Union[int, str, IO[Any]]
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using TypeAlias can make the intentions of type aliases clearer and improve static type checking. This change is beneficial for code clarity and maintainability, especially in a shared codebase.

    6
    Use more specific type hints to improve code clarity and catch potential type-related issues

    Consider using a more specific type hint for the actions parameter in the
    _convert_keys method. Instead of Dict[str, Any], you could use Dict[str, Union[str,
    int, float, None]] if those are the expected types.

    py/selenium/webdriver/common/actions/pointer_input.py [73]

    -def _convert_keys(self, actions: Dict[str, Any]):
    +def _convert_keys(self, actions: Dict[str, Union[str, int, float, None]]):
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use more specific type hints for the actions parameter in the _convert_keys method can improve code clarity and help catch potential type-related issues. However, it assumes knowledge of the expected types, which may not be accurate without further context.

    5
    Use a more specific type hint for the capabilities dictionary to improve type checking and documentation

    Consider importing TypedDict from typing instead of Dict for more precise type
    hinting of the default_capabilities method return value. This can provide better
    type checking and documentation of the expected structure.

    py/selenium/webdriver/wpewebkit/options.py [18-65]

    -from typing import Dict
    +from typing import TypedDict
     
     ...
     
    +class WPEWebKitCapabilities(TypedDict):
    +    browserName: str
    +    # Add other capability keys as needed
    +
     @property
    -def default_capabilities(self) -> Dict[str, str]:
    +def default_capabilities(self) -> WPEWebKitCapabilities:
         return DesiredCapabilities.WPEWEBKIT.copy()
    Suggestion importance[1-10]: 5

    Why: The suggestion to use TypedDict instead of Dict for the default_capabilities method return value can enhance type checking and documentation. However, it requires defining a new class and may not be necessary if the dictionary structure is simple or unlikely to change.

    5
    Implement a more specific type hint for the capabilities dictionary to enhance type checking and documentation

    Similar to the WPEWebKit options, consider using TypedDict for more precise type
    hinting of the default_capabilities method return value. This can provide better
    type checking and documentation of the expected structure for Safari capabilities.

    py/selenium/webdriver/safari/options.py [17-115]

    -from typing import Dict
    +from typing import TypedDict
     
     ...
     
    +class SafariCapabilities(TypedDict):
    +    browserName: str
    +    # Add other capability keys as needed
    +
     @property
    -def default_capabilities(self) -> Dict[str, str]:
    +def default_capabilities(self) -> SafariCapabilities:
         return DesiredCapabilities.SAFARI.copy()
    Suggestion importance[1-10]: 5

    Why: Similar to the previous suggestion, using TypedDict for the default_capabilities method return value can improve type checking and documentation. It involves additional complexity, which may not be justified if the dictionary structure is straightforward.

    5
    Refine the type hint for console log arguments to better represent their possible structures

    Consider using a more specific type hint for the args attribute in the
    ConsoleLogEntry class. Instead of List[dict], you could use List[Any] or create a
    custom type that better represents the structure of the console log arguments.

    py/selenium/webdriver/common/bidi/script.py [76-79]

    +from typing import Any, List
    +
     @dataclass
     class ConsoleLogEntry:
         text: str
         timestamp: str
         method: str
    -    args: List[dict]
    +    args: List[Any]
         type_: str
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion to use List[Any] instead of List[dict] for the args attribute in ConsoleLogEntry is valid but offers marginal improvement. It could be beneficial if the structure of args is not strictly a dictionary, but it reduces specificity.

    4

    💡 Need additional feedback ? start a PR chat


    Previous suggestions

    Suggestions up to commit d792fcf
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Specify the types of keys and values in the env parameter

    Specify the types of keys and values in the env parameter of the Service constructor
    to enhance type safety.

    py/selenium/webdriver/common/service.py [57]

    -env: Optional[Mapping[Any, Any]] = None,
    +env: Optional[Mapping[str, str]] = None,
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly specifies the types of keys and values in the env parameter, enhancing type safety and clarity.

    10
    Specify dictionary types in a list for enhanced type safety

    Specify the type of dictionary in the args list to enhance type safety and clarity
    in the codebase.

    py/selenium/webdriver/common/bidi/script.py [81]

    -args: List[dict]
    +args: List[Dict[str, Any]]
     
    Suggestion importance[1-10]: 8

    Why: Specifying the type of dictionary in the args list enhances type safety and clarity, making the codebase more robust and easier to understand.

    8
    Possible bug
    Add a type hint to executable_path for consistency and clarity

    Ensure that executable_path has a consistent type hint across all service
    initializers for better type consistency and to avoid potential bugs.

    py/selenium/webdriver/chrome/service.py [38]

    -executable_path=None,
    +executable_path: Optional[str] = None,
     
    Suggestion importance[1-10]: 9

    Why: Adding a type hint to executable_path ensures type consistency across service initializers, which can help avoid potential bugs and improve code clarity.

    9
    Maintainability
    Combine multiple imports from the same module into a single line

    Combine the imports from the same module (typing) into a single line to improve
    readability and maintain consistency.

    py/selenium/webdriver/edge/service.py [19-21]

    -from typing import List
    -from typing import Mapping
    -from typing import Optional
    +from typing import List, Mapping, Optional
     
    Suggestion importance[1-10]: 7

    Why: Combining imports from the same module into a single line improves readability and maintains consistency. However, it is a minor improvement and does not address any critical issues.

    7

    @VietND96 VietND96 added the C-py label Nov 1, 2024
    @VietND96
    Copy link
    Member

    VietND96 commented Nov 1, 2024

    Hi, can you resolve the conflicts due to recently changes?

    @iampopovich
    Copy link
    Contributor Author

    yepp . i'll do it until tomorrow morning

    @iampopovich
    Copy link
    Contributor Author

    I decided to start again from the trunk's HEAD 🙏
    There are quite a few conflicts here that I don’t want to resolve incorrectly.

    @iampopovich iampopovich marked this pull request as draft November 2, 2024 13:07
    @iampopovich iampopovich marked this pull request as ready for review November 2, 2024 14:12
    @iampopovich
    Copy link
    Contributor Author

    @VietND96 cc

    Copy link
    Contributor

    Persistent review updated to latest commit 597f8f9

    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