Skip to content

[py] accept By in find_element/find_elements type hints - #17870

Merged
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:py-by-type-hints
Aug 4, 2026
Merged

[py] accept By in find_element/find_elements type hints#17870
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:py-by-type-hints

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Restores type-checker compatibility for find_element / find_elements locator arguments

🔧 Implementation Notes

  • Added By to the accepted union so str, ByType, custom/Appium strategies, and By all keep type-checking.
  • Alternatives considered:
    • Narrowing back to ByType would re-break the Appium
    • making By a str subclass would force By.ID to one static type and break whichever of by: By / by: ByType it didn't match.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the type-hint widening across the three remote modules
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 4, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Python: Accept By in find_element/find_elements type hints

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Restore type-checker compatibility for By.ID passed to find_element(s).
• Widen locator argument unions across WebDriver, WebElement, and ShadowRoot.
• Preserve compatibility with custom/Appium string strategies and RelativeBy locators.
Diagram

graph TD
  A["Client code"] --> B["WebDriver.find_element(s)"] --> F["Remote command execute"]
  A --> C["WebElement.find_element(s)"] --> D["Locator converter"] --> F
  A --> E["ShadowRoot.find_element(s)"] --> F
  G["Locator arg (str | By | RelativeBy)"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce a shared `ByArg` TypeAlias
  • ➕ Avoids repeating unions across modules (str | By | RelativeBy).
  • ➕ Makes future locator-type expansions a single-line change.
  • ➖ Requires choosing an import location (public vs internal typing module).
  • ➖ Slightly larger refactor surface than a direct signature tweak.
2. Use overloads for different locator families
  • ➕ Can express more precise typing per locator strategy (e.g., RelativeBy vs By vs str).
  • ➕ Improves IDE help for common call patterns.
  • ➖ More verbose and harder to maintain across multiple entry points.
  • ➖ Risk of diverging overload sets between WebDriver/WebElement/ShadowRoot.

Recommendation: The chosen approach (widening the locator union to include By) is the most pragmatic fix: it restores static-type compatibility without changing runtime behavior or constraining Appium/custom string strategies. If follow-up cleanup is desired, consider a shared ByArg TypeAlias to reduce duplication, but it’s not required for correctness.

Files changed (3) +6 / -6

Bug fix (3) +6 / -6
shadowroot.pyAllow 'By' in ShadowRoot 'find_element(s)' type hints +2/-2

Allow 'By' in ShadowRoot 'find_element(s)' type hints

• Widen the 'by' parameter type from 'str' to 'str | By' for both 'find_element' and 'find_elements'. This restores type-checker acceptance of passing 'By.ID' (and other 'By' members) directly.

py/selenium/webdriver/remote/shadowroot.py

webdriver.pyAccept 'By' alongside 'RelativeBy' in WebDriver 'find_element(s)' +2/-2

Accept 'By' alongside 'RelativeBy' in WebDriver 'find_element(s)'

• Extend the 'by' parameter union from 'str | RelativeBy' to 'str | By | RelativeBy' for 'find_element' and 'find_elements'. This keeps RelativeBy support while restoring compatibility for 'By.*' locators under static type checking.

py/selenium/webdriver/remote/webdriver.py

webelement.pyAllow 'By' in WebElement 'find_element(s)' type hints +2/-2

Allow 'By' in WebElement 'find_element(s)' type hints

• Widen the 'by' parameter type from 'str' to 'str | By' for both 'find_element' and 'find_elements'. This aligns WebElement typing with common usage ('By.ID', etc.) while preserving string-based custom strategies.

py/selenium/webdriver/remote/webelement.py

@qodo-code-review

qodo-code-review Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Misleading By type hint ✗ Dismissed 🐞 Bug ⚙ Maintainability
Description
find_element/find_elements now annotate by as including By, but in this codebase By is a
namespace class whose locator constants are ByType string literals, not By instances. This
widens typing to allow By() instances to type-check even though they are forwarded as the
WebDriver payload using field and can fail JSON serialization at request time.
Code

py/selenium/webdriver/remote/webdriver.py[887]

+    def find_element(self, by: str | By | RelativeBy = By.ID, value: str | None = None) -> WebElement:
Evidence
By is defined as a plain class holding string-literal constants (ByType), so including By in
the locator unions does not model real locator values. The WebDriver code forwards by directly
into the command params as using, and RemoteConnection.execute JSON-serializes those params;
therefore, allowing By instances via typing can lead to runtime JSON serialization failures.

py/selenium/webdriver/common/by.py[23-97]
py/selenium/webdriver/remote/webdriver.py[887-912]
py/selenium/webdriver/remote/remote_connection.py[384-407]
py/selenium/webdriver/remote/utils.py[22-24]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The locator argument type hints were widened to include `By` (e.g., `by: str | By` / `by: str | By | RelativeBy`), but `By` in this repo is a namespace container of string constants (`ByType`), not the runtime type of locator values. This makes invalid inputs like `By()` appear type-safe while they will be forwarded as the wire-protocol `using` value.

## Issue Context
`find_element`/`find_elements` ultimately send `{ "using": by, "value": value }` through `RemoteConnection.execute`, which JSON-encodes params. Non-string objects (e.g., `By()` instances) are not JSON serializable.

## Fix Focus Areas
- py/selenium/webdriver/remote/webdriver.py[887-943]
- py/selenium/webdriver/remote/webelement.py[510-556]
- py/selenium/webdriver/remote/shadowroot.py[55-123]

### Suggested change
- Import and use `ByType` in annotations instead of `By`:
 - `ShadowRoot`/`WebElement`: `by: str | ByType = By.ID`
 - `WebDriver`: `by: str | ByType | RelativeBy = By.ID`
- Keep `str` in the union to support custom/Appium strategies, and keep `RelativeBy` where applicable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread py/selenium/webdriver/remote/webdriver.py

@navin772 navin772 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@titusfortner
titusfortner merged commit 778cc7a into SeleniumHQ:trunk Aug 4, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [py] type errors for ByType in fing_element / find_elements in webdriver.py

4 participants