Make SearchContext.findElements have a generic return type#9953
Make SearchContext.findElements have a generic return type#9953shs96c wants to merge 2 commits intoSeleniumHQ:trunkfrom
SearchContext.findElements have a generic return type#9953Conversation
shs96c
force-pushed
the
generic-find-elements
branch
from
d0a70bb to
385a893
Compare
| * @see org.openqa.selenium.By | ||
| */ | ||
| List<WebElement> findElements(By by); | ||
| <T extends WebElement> List<T> findElements(By by); |
There was a problem hiding this comment.
should a similar change be done for findElement?
There was a problem hiding this comment.
No, because we can rely on co-variant return types there. The only signature that needs to change is findElements, and that's because we want to return a typed list, and List<WebElement> is not the same as List<? extends WebElement> in generics.
titusfortner
left a comment
titusfortner
left a comment
There was a problem hiding this comment.
If we don't see any backward compatibility issues, then we should merge it. The sooner we release this, the sooner the appium devs can use it for their next release.
| */ | ||
| @Override | ||
| List<WebElement> findElements(By by); | ||
| <T extends WebElement> List<T> findElements(By by); |
There was a problem hiding this comment.
This change should also probably go into the corresponding method of the abstract By class?
There was a problem hiding this comment.
Good point. I'll do that too.
Why do this? The main reason is to support frameworks that wish to
extend Selenium, but return their own sub-types of `WebElement` from
`findBy`.
`findElement` can already be overridden using co-variant return types,
but the `List<WebElement>` returned by `findElements` could not be
overridden as easily. This signature change allows that.
With this change in place, subclasses of WebDriver can now do
something like this, without there being compilation issues:
```
public interface Special extends WebDriver {
SpecialElement findElement(By locator);
List<Specialelement> findElements(By locator);
}
```
Compiling the entire java code base suggests that this is a safe
change to make -- no tests or other calls sites needed to be modified
in order for this change to work.
shs96c
dismissed stale reviews from diemol, titusfortner, and mykola-mokhnach
via
d9816da
shs96c
force-pushed
the
generic-find-elements
branch
from
385a893 to
d9816da
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
While this change “only” causes some warnings in Java (which can be easily suppressed), this change will break Kotlin code, and that’s far from ideal. Perhaps we need to hold off on this until we come up with a way to make this work smoothly for Kotlin users. |
|
After chatting with @mykola-mokhnach and checking the linked PR, the Appium team does not need this anymore. |
4 participants
Why do this? The main reason is to support frameworks that wish to
extend Selenium, but return their own sub-types of
WebElementfromfindBy.findElementcan already be overridden using co-variant return types,but the
List<WebElement>returned byfindElementscould not beoverridden as easily. This signature change allows that.
With this change in place, subclasses of WebDriver can now do
something like this, without there being compilation issues:
Compiling the entire java code base suggests that this is a safe
change to make -- no tests or other calls sites needed to be modified
in order for this change to work.
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
Motivation and Context
Types of changes
Checklist