Skip to content

Conversation

@shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Nov 15, 2024

User description

Description

deprecated cdp for firefox

Motivation and Context

Shift towards pure BiDi

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

  • Added deprecation warnings to multiple functions in the cdp.py file, indicating that these functions are deprecated for Firefox.
  • Encouraged users to migrate to the new BiDi implementations by using warnings.warn with DeprecationWarning.
  • Functions affected include import_devtools, get_connection_context, get_session_context, connection_context, session_context, set_global_connection, set_global_session, execute, listen, wait_for, _handle_data, _handle_cmd_response, _handle_event, dom_enable, page_enable, aclose, open_session, connect_session, _reader_task, open_cdp, and connect_cdp.

Changes walkthrough 📝

Relevant files
Enhancement
cdp.py
Deprecate CDP functions for Firefox with warnings               

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

  • Added deprecation warnings for several functions related to CDP for
    Firefox.
  • Encouraged migration to new BiDi implementations.
  • Used warnings.warn to notify users of deprecated functions.
  • +91/-1   

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

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    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

    Code Duplication
    The deprecation warning messages are duplicated across multiple functions with only the function name changing. Consider extracting the warning logic into a helper function to reduce duplication.

    Documentation Gap
    The deprecation warnings should include information about which specific BiDi implementations users should migrate to, rather than just a generic message to migrate.

    Incorrect Warning
    The warning message for _reader_task() incorrectly refers to 'render_task()' instead of '_reader_task()' in the deprecation message.

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Consolidate multiple identical deprecation warnings into a single module-level warning to reduce warning noise

    Move the deprecation warning to the module level to avoid repeated warnings for each
    function call. This will reduce noise while still effectively communicating the
    deprecation.

    py/selenium/webdriver/common/bidi/cdp.py [51-58]

    +# At module level
    +warnings.warn("CDP functionality is deprecated for Firefox. Please migrate to the new BiDi implementations",
    +             DeprecationWarning, stacklevel=2)
    +
     def import_devtools(ver):
    -    warnings.warn(
    -        "import_devtools() is now deprecated for Firefox. Please migrate to the new BiDi implementations", 
    -        DeprecationWarning, 
    -        stacklevel=2)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Moving the warning to module level would significantly reduce warning noise while maintaining the deprecation notice. This is particularly important given the large number of deprecated functions in this module.

    8
    Typo
    Fix a typo in a deprecation warning message

    The warning message in _handle_data() has a typo in the function name ("handle_date"
    instead of "handle_data").

    py/selenium/webdriver/common/bidi/cdp.py [308-311]

     warnings.warn(
    -    "handle_date() is now deprecated for Firefox. Please migrate to the new BiDi implementations", 
    +    "handle_data() is now deprecated for Firefox. Please migrate to the new BiDi implementations", 
         DeprecationWarning, 
         stacklevel=2)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The typo in the warning message ("handle_date" instead of "handle_data") could confuse users. Fixing it improves the clarity and professionalism of the deprecation messages.

    7
    Correct an incorrect function name in a deprecation warning message

    The warning message in _reader_task() incorrectly refers to "render_task" instead of
    "reader_task".

    py/selenium/webdriver/common/bidi/cdp.py [512-515]

     warnings.warn(
    -    "render_task() is now deprecated for Firefox. Please migrate to the new BiDi implementations", 
    +    "reader_task() is now deprecated for Firefox. Please migrate to the new BiDi implementations", 
         DeprecationWarning, 
         stacklevel=2)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The warning message incorrectly refers to "render_task" instead of "reader_task", which could mislead users. Fixing this improves the accuracy of the deprecation message.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 added the C-py Python Bindings label Nov 19, 2024
    @VietND96 VietND96 changed the base branch from trunk to cdp-firefox November 21, 2024 13:48
    @VietND96 VietND96 merged commit 49148e6 into SeleniumHQ:cdp-firefox Nov 21, 2024
    1 check passed
    VietND96 added a commit that referenced this pull request Nov 21, 2024
    @shbenzer shbenzer deleted the deprecate-firefox-cdp branch January 16, 2025 00:38
    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.

    2 participants