Skip to content

[py] Fix mypy type annotation issues in action_builder #16207

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

[py] Fix mypy type annotation issues in action_builder #16207

wants to merge 8 commits into from

Conversation

pallavigitwork
Copy link
Member

@pallavigitwork pallavigitwork commented Aug 19, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

💥 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
@qodo-merge-pro Qodo Merge Pro
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)]

@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 19, 2025 •
edited
Loading

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
cgoldberg
cgoldberg requested changes Aug 19, 2025
View reviewed changes
Copy link
Contributor

@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.

navin772
navin772 reviewed Aug 20, 2025
View reviewed changes
navin772
navin772 approved these changes Aug 21, 2025
View reviewed changes
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!

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

@cgoldberg cgoldberg cgoldberg requested changes

@navin772 navin772 navin772 approved these changes

Assignees
No one assigned
Labels
C-py Python Bindings Review effort 2/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pallavigitwork @cgoldberg @navin772 @selenium-ci