Skip to content

[py] fixed a bug in bidi/session.py by removing mutable object as default value for function argument.#14286

Merged
p0deje merged 2 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:bug-fix
Oct 3, 2024
Merged

[py] fixed a bug in bidi/session.py by removing mutable object as default value for function argument.#14286
p0deje merged 2 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:bug-fix

Conversation

@sandeepsuryaprasad
Copy link
Copy Markdown
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jul 20, 2024 •
edited
Loading

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

  • Generally it is not a good idea to have mutable objects as default values for function arguments.
  • In functions session_subscribe and session_unsubscribe, the function parameter browsing_contexts had a mutable object as default value.
  • In this PR I have defaulted browsing_contexts to None as default value to the parameter.

Motivation and Context

It is not recommended/safe to assign mutable objects as default values for function parameters.

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

  • Fixed a bug in bidi/session.py by changing the default value of the browsing_contexts parameter to None in the session_subscribe and session_unsubscribe functions to avoid using mutable objects as default values.
  • Added checks to initialize browsing_contexts as an empty list if it is None in the session_subscribe and session_unsubscribe functions.
  • Fixed linting issues in support/relative_locator.py by adjusting the formatting of @overload method definitions.

Changes walkthrough 📝

Relevant files
Bug fix
session.py
Fix mutable default argument in session_subscribe and
session_unsubscribe

py/selenium/webdriver/common/bidi/session.py

  • Changed default value of browsing_contexts parameter to None in
    session_subscribe and session_unsubscribe functions.
  • Added checks to initialize browsing_contexts as an empty list if it is
    None.
    		•  +6/-2     
    Enhancement
    relative_locator.py
    Fix linting issues in `relative_locator.py`                           

    py/selenium/webdriver/support/relative_locator.py

  • Fixed linting issues by adjusting the formatting of @overload method
    definitions.
    		•  +20/-10 

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

    @qodo-code-review qodo-code-review Bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue Review effort [1-5]: 2 labels Jul 20, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Jul 20, 2024

    PR Reviewer Guide 🔍

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

    Code Improvement
    The change from a mutable default argument to None with a conditional initialization to an empty list is a good practice to avoid potential bugs related to mutable default arguments.

    Code Improvement
    Similar to the previous function, this change also improves the function definition by removing a mutable default argument.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Jul 20, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for the distance parameter to ensure it is non-negative

    For the near method, consider validating the distance parameter to ensure it is a
    non-negative integer. This validation can prevent runtime errors and enforce that
    the distance parameter adheres to expected values.

    py/selenium/webdriver/support/relative_locator.py [171]

     def near(self, element_or_locator: Union[WebElement, LocatorType], distance: int = 50) -> "RelativeBy":
+    if not isinstance(distance, int) or distance < 0:
+        raise ValueError("Distance must be a non-negative integer")
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding validation for the distance parameter is a crucial improvement that prevents potential runtime errors and ensures the parameter adheres to expected values.

    		9
    Best practice
    Use immutable default arguments where possible

    To avoid potential issues with mutable default arguments, consider using a tuple for
    events if the function does not modify this parameter. This change ensures that the
    default argument is not only immutable but also safe from unintended side-effects.

    py/selenium/webdriver/common/bidi/session.py [19]

    -def session_subscribe(*events, browsing_contexts=None):
+def session_subscribe(*events: tuple, browsing_contexts=None):
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a tuple for events ensures immutability and avoids potential issues with mutable default arguments. However, it is a minor improvement since the function does not modify events.

    		7
    Maintainability
    Simplify method overloads by removing redundant default None

    The above method's overload can be simplified by removing the unnecessary None
    default for element_or_locator in the second overload, as it does not add value and
    the main method already handles None as a default.

    py/selenium/webdriver/support/relative_locator.py [95]

    -def above(self, element_or_locator: None = None) -> "NoReturn":
+def above(self, element_or_locator: None) -> "NoReturn":
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Removing the redundant default None simplifies the overload but does not significantly impact functionality or readability.

    		5

    @sandeepsuryaprasad
    Copy link
    Copy Markdown
    Contributor Author

    sandeepsuryaprasad commented Aug 23, 2024

    @diemol can you please trigger the workflow for this PR as well. Thanks!

    @sandeepsuryaprasad
    Copy link
    Copy Markdown
    Contributor Author

    sandeepsuryaprasad commented Sep 3, 2024

    @diemol can you please trigger the workflow for this PR..

    @sandeepsuryaprasad
    Copy link
    Copy Markdown
    Contributor Author

    sandeepsuryaprasad commented Oct 1, 2024

    @diemol @AutomatedTester @titusfortner @pujagani can someone trigger the workflow for this PR please.. it's been more than couple of months since I raised it.. Thanks!

    @pujagani pujagani requested a review from p0deje October 3, 2024 05:17
    @p0deje p0deje merged commit e8820b5 into SeleniumHQ:trunk Oct 3, 2024
    @sandeepsuryaprasad sandeepsuryaprasad deleted the bug-fix branch November 24, 2024 04:14
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @sandeepsuryaprasad @M1troll
    [py] fixed a bug in bidi/session.py by removing mutable object as d…
    88a67b9 
    …efault value for function argument. (SeleniumHQ#14286)
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @p0deje p0deje p0deje approved these changes

    Assignees

    No one assigned

    Labels

    P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @sandeepsuryaprasad @p0deje @pujagani