Skip to content
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

Removed findelement by nsPredicate string #394

Merged
merged 1 commit into from
May 24, 2016
Merged

Removed findelement by nsPredicate string #394

merged 1 commit into from
May 24, 2016

Conversation

SrinivasanTarget
Copy link
Member

@SrinivasanTarget SrinivasanTarget commented May 21, 2016

Change list

1)Removed findelement by nsPredicate string
2)findelement and findelements methods of RemoteWebdriver is now public

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

1)Removed findelement by nsPredicate string
@Rafael-Chavez Apologies for removing this now.#352 When XCUIT mode will work then we will commit these changes again with same authorship(rafael-chavez).
2)findelement and findelements methods of RemoteWebdriver is now public
Above methods are made public so that consumers of JSONWP in other projects can utilise this easily.

As we discussed earlier, IOSDriver will be split into IOSAutomationDriver and IOSXCUITDriver in upcoming PR's soon.

@TikhomirovSergey @Rafael-Chavez Please review.

@@ -228,6 +228,14 @@ private static CommandInfo deleteC(String url) {
return super.findElementsById(id);
}

@Override public T findElement(String by, String using) {
Copy link
Contributor

@TikhomirovSergey TikhomirovSergey May 21, 2016

Choose a reason for hiding this comment

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

It is not necessary to add @OverRide public T findElement(String by, String using)
but it is neccessary to override public List findElements(String by, String using) with T

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on it.Will close this and styling issues ASAP.

@rafe-g
Copy link
Contributor

rafe-g commented May 23, 2016

@SrinivasanTarget, what do you mean by 'XCUIT mode will work'? Currently appium-xcuitest-driver does work with when used in conjunction with the nspredicate changes in the java-client.

@SrinivasanTarget
Copy link
Member Author

SrinivasanTarget commented May 23, 2016

@Rafael-Chavez We accept nspredicate work but other UIAutomation methods won't work now on xcuitest-driver as per current status/architecure of java-client repo.So as you are aware iOSDriver is going to be split into IOSXCUITDriver and IOSAutomationDriver soon.Once this work is done, we will get this back to repo again with same authorship.

#352 will be in again once driver split up is done.
Thanks for your support.

@rafe-g
Copy link
Contributor

rafe-g commented May 23, 2016

Great. Looking forward to the Split up driver work. Thanks @SrinivasanTarget

@SrinivasanTarget
Copy link
Member Author

Thanks for your support @Rafael-Chavez . Split up will happen soon in upcoming PR's.

@SrinivasanTarget
Copy link
Member Author

@TikhomirovSergey Have fixed the remarks

@@ -224,6 +224,10 @@ private static CommandInfo deleteC(String url) {
return super.findElements(by);
}

@Override public List findElements(String by, String using) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@SrinivasanTarget
You forgot to change this string to

@Override public List<T> findElements(String by, String using)

@TikhomirovSergey
Copy link
Contributor

Everything is ok. But there is the remark above.

Override findelements method in AppiumDriver

fixed code styling issues

minor corrections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants