Skip to content

Conversation

pallavigitwork
Copy link
Member

@pallavigitwork pallavigitwork commented Aug 19, 2025

User description

🔗 Related Issues

#15697

💥 What does this PR do?

fixed mypy issues in action builder file

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Other


Description

  • Fixed mypy type checking issues in action builder

  • Added explicit type annotations for class attributes

  • Replaced type checking with isinstance() calls

  • Reorganized imports for better type hint support


Diagram Walkthrough

flowchart LR
  A["Import reorganization"] --> B["Type annotations added"]
  B --> C["isinstance() checks"]
  C --> D["Mypy compliance"]
Loading

File Walkthrough

Relevant files
Bug fix
action_builder.py
Fix mypy type checking issues                                                       

py/selenium/webdriver/common/actions/action_builder.py

  • Moved typing imports to end of import section
  • Added explicit type annotations for devices and enc variables
  • Replaced string-based type checking with isinstance() calls
  • Enhanced type safety for mypy compliance
+6/-5     

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 19, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Investigate and resolve "ConnectFailure" with multiple ChromeDriver instances.
  • Ensure subsequent instances do not log connection errors.

Requires further human verification:

  • Reproduce the issue across environments and ChromeDriver versions to validate any fix.

1234 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Ensure click() triggers JS in href for Firefox as per regression report.

Requires further human verification:

  • Cross-browser manual test to confirm JS execution on click in Firefox.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

get_device_with compares device objects to a string name; this likely always fails and may change behavior. Verify expected comparison (e.g., comparing device.name or id).

Returns:
--------
Optional[Union[WheelInput, PointerInput, KeyInput]] : The device with the given name.
"""
return next(filter(lambda x: x == name, self.devices), None)
Behavior Change

pointer_inputs/key_inputs now use isinstance checks instead of comparing device.type to interaction constants. Confirm all device classes inherit as expected and that no proxies/mocks rely on .type filtering.

def pointer_inputs(self) -> list[PointerInput]:
    return [device for device in self.devices if isinstance(device, PointerInput)]

@property
def key_inputs(self) -> list[KeyInput]:
    return [device for device in self.devices if isinstance(device, KeyInput)]
Typing Compatibility

Use of built-in generics list[...] requires Python 3.9+. Ensure project supports this or switch to from typing import List for broader compatibility.

def pointer_inputs(self) -> list[PointerInput]:
    return [device for device in self.devices if isinstance(device, PointerInput)]

@property
def key_inputs(self) -> list[KeyInput]:
    return [device for device in self.devices if isinstance(device, KeyInput)]

Copy link
Contributor

qodo-merge-pro bot commented Aug 19, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Keep type hints consistent

Use the already imported classes without repeating them here to keep the union
minimal and future-proof. Referencing the concrete imported classes is fine, but
avoid string annotations or redundant qualification.

py/selenium/webdriver/common/actions/action_builder.py [44]

+self.devices: List[Union[PointerInput, KeyInput, WheelInput]] = [mouse, keyboard, wheel]
 
-
  • Apply / Chat
Suggestion importance[1-10]: 1

__

Why: This suggestion is correct but offers no improvement, as the existing_code and improved_code are identical; it merely affirms that the current code follows good practice.

Low
  • Update

@cgoldberg cgoldberg changed the title fixed mypy issues in action builder file [py] Fix mypy type annotation issues in action_builder Aug 19, 2025
Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

You need to move the import line so the linter doesn't fail.

If you can run ./scripts/format.sh it will probably do it for you.

@pallavigitwork
Copy link
Member Author

Ok.
Thanks.
Sorry I forgot that step before raising the pr.

Copy link
Member

@navin772 navin772 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pallavigitwork
Copy link
Member Author

@cgoldberg / @navin772 , is there anything pending in the PR for merging? Please let me know if its accepted for merge? Thanks.

@navin772
Copy link
Member

It is good to go from my side, waiting for Corey to review it once.

@pallavigitwork
Copy link
Member Author

ok thanks @navin772 .

Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

LGTM.. thanks!

@cgoldberg cgoldberg merged commit ed794f7 into SeleniumHQ:trunk Aug 29, 2025
4 checks passed
@pallavigitwork
Copy link
Member Author

Thanks @cgoldberg .

@pallavigitwork pallavigitwork deleted the 15697Fix branch September 8, 2025 10:39
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.

4 participants