-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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.
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
Java: All searching interfaces are made generic #863
Conversation
Ok guys, thanks for remarks |
can't assume @alexec has, since he never mentioned it in the other PR and hasn't had a commit merged in the selenium tree. |
duplicating is the same as "copying" and since alexec's employer could potentially claim copyright on that code if we 'copied' since he hasn't signed the CLA.... then no, you can't duplicate it. And I would then suggest you remove his commits from your tree. That being said, it would then be nice to have an additional test ;) |
Ok. So I'll make improvements that are required soon... |
Hi everyone, I'm not sure if I've signed the CLA. If you really need to me too, then just let me know. |
@alexec Yeah, we really need you. Btw. Can you squash your commits? |
CLA signed. I'm afraid I don't know how to squash commits. |
@alexec Please try something like that: git reset --soft head~8 |
I got weird merge issue. I don't know how to do this and it'll take me ages to figure out. Do you think you could do it please? |
ok. I'll try this later... |
c3ab8ef
to
db7fdb8
Compare
1b054cc
to
db7fdb8
Compare
cbce6e8
to
48453b0
Compare
Oh my god! There were problems with the merging... I'll add soon all that was required |
There were problems with the merging... So the new commit was delayed What I've done:
<T extends WebElement> List<T> findElements
<T extends WebElement> T findElement The same way were enhanced WebElement and WebDriver. Their common implementations work as usual. There shouldn't be backward compatibility issues.
Actually I don't know what test I can make up. All this project is like a test stand for this change. |
No further comments on the actual code. At the design level, I'm not sure how I feel about the lack of type safety this introduces:
This is now perfectly valid (and I guess the point), but the |
@jleyba Ok. Does it makes sense to declare the following: <T extends WebElement> List<T> findElements(By by) throws ClassCastException;
<T extends WebElement> T findElement(By by) throws ClassCastException; ? Actially this enhancement is supposed to be used when there is need to implement user's customized WebDriver/WebElement. I think that it makes sense to point these special things out at documentation if this is supposed to be merged... |
6381978
to
a140a04
Compare
a140a04
to
48453b0
Compare
004a6fb
to
ad00dd5
Compare
The problem: There are many Selenium-based projects such as Appium (java_client), Selendroid, IOSDriver and so on. They are used not only for the web/browser testing and they have their own WebElement implementations. Also there is probably need to re-implement all changed interfaces for some end user purposes. Current design doesn't allow user to use their own WebElement subclasses without additional casting. It is annoying and makes code dirty. Any trying to avoid this causes problems. This change is backward compatible with existing projects and will allow to create factories or user's customized Webdrivers and WebElements that return desired WebElement subclasses.
@ddavison |
* @return A list of all {@link WebElement}s, or an empty list if nothing matches | ||
* | ||
* @throws ClassCastException The WebDriver interface is generic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments about ClassCastException
unnecessary? ClassCastException
is unchecked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexec this is just information. If end users will re-implement the proposed API then there won't be any problem. An end user will face a problem when they use common Selenium implementations, because it not so flexible like API.
Why is there a need to make the interfaces generic?
My only issue is with the The following will break without compile error or warning if you change the implementation of the webdriver:
Or am I missing something? |
@squallified There are some problems:
and
There shouldn't be any compilation problem if API will be changed the proposed way. As you see all tests have passed. There are possible "warnings" when you are trying to re-implement the proposed API forcing finders (WebDriver/WebElement) to return your own WebElement subclass.
This is just an attempt to make API a bit more convenient and flexible. 😄 |
You can fix the first example by changing the return type of the findElement method in your subclass:
This doesn't work for the findElements method though. For this to work the method signature in the interface needs to be changed to:
I agree that findElements needs to be changed (either generic interface or a wildcard). I guess it's just a preference thing. |
@squallified So the proposed PR allows end user to use API and it don't force user to use only certain implementations if their project contains a customized WebElement implementation which their own WebDrivers return: WebDriver d;
CustomWebElement = d.findElement(By.id("test")) //works with these changes
List<CustomWebElement> = d.findElements(By.id("test")) //works with these changes So it is one more advantage of proposed changes. |
Following |
👍 |
I prefer @squallified 's suggestion of changing findElements to |
@jleyba |
@jleyba The simple sample:
As you can see the first class was extended without problems. Ok. Lets go ahead. but The same is true for our case. I was able to build API artefact. There were lots of compilation errors. And now we can see: Summary: So I'll keep this PR as is. Advantages:
Disadvantages:
Related threads where the mentioned wildcard problem was discussed: |
It is possible that proposed code will be changed the way below: public interface SearchContext<T extends WebElement> {
List<T> findElements(By by);
T findElement(By by);
}
public interface Webdriver<T extends WebElement> extends SearchContext<T> {
...
List<T> findElements(By by);
T findElement(By by);
...
}
public class RemoteWebDriver implements Webdriver<WebElement> {
...
} How do you feel about that change? It alows to avoid API contract problems but users will face rawtype warning at the first time. I think it is very major change... |
Hello there, Relevant discussion on the appium project - appium/java-client#1021 |
Venkata, your tone of voice in your comment is entitled. Please read what you write before you post on open source projects. Your issue, shouldn't be an issue at all for the selenium project and you'll need to follow up with the appium developers. Please read my comment on this other issue raised and referenced in this PR: As it stands, I think this PR might never get merged (it currently has merge conflicts too). I've also come to dislike the style of generics used here as it can cause many unintended side effects and makes for refactoring code a very difficult chore to unwind. |
Luke, Yes, I checked with appium devs. They sounded like the PR is pending with selenium that is why we have to duplicate the classes in appium. |
|
Apologies for the poor handling of this PR by our side, nevertheless it looks like the approach was never fully accepted. On the other hand, I can see that the purpose of this PR was achieved in the end through the implementation of generics in the Java bindings for Appium, in this PR appium/java-client#413. |
The problem:
There are many Selenium-based projects such as Appium (java_client),
Selendroid, IOSDriver and so on. They are used not only
for the web/browser testing and they have their own WebElement
implementations. Also there is probably need to re-implement all changed
interfaces for some end user purposes.
Current design doesn't allow user to use their own WebElement subclasses
without additional casting. It is annoying and makes code dirty. Any
trying to avoid this causes problems.
This change is backward compatible with existing projects and
will allow to create factories or user's customized Webdrivers and
WebElements that return desired WebElement subclasses.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)