refactor: unify locator factories naming and `toString` implementations #1538

valfirst · 2021-10-21T08:17:38Z

Change list

Keep the unified naming of methods and toString implementations.

Types of changes

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

No changes in production code.
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

Unify locator factory-methods naming and toString implementations to follow the same approach as it's done in Selenium.

mykola-mokhnach · 2021-10-21T08:48:29Z

I'm not quite sure I want to lend this change. It would only create more hassle for end users without giving any actual benefits

valfirst · 2021-10-21T08:50:04Z

@mykola-mokhnach ok, that's up to you, feel free to close the PR

mykola-mokhnach · 2021-10-21T18:28:52Z

@SrinivasanTarget What do you think regarding the above?

mykola-mokhnach · 2021-10-21T19:26:10Z

We could do it this way - old locators stay, but get @Deprecated annotation with the recommendation to use the fixed ones, so users could have a chance to update their scripts until we remove the deprecated stuff completely

mykola-mokhnach · 2021-10-21T19:27:22Z

I was also thinking about renaming MobileBy to AppiumBy. MobileBy makes sense for iOS and Android, but does not make sense for mac and windows and also other non-mobile drivers, so we need some more generic name. The renaming strategy could be exactly the same as described above

src/main/java/io/appium/java_client/MobileBy.java

saikrishna321 · 2021-10-22T00:41:56Z

I like the idea to add Deprecate tag and also rename to AppiumBy

SrinivasanTarget · 2021-10-22T01:55:04Z

I was also thinking about renaming MobileBy to AppiumBy. MobileBy makes sense for iOS and Android, but does not make sense for mac and windows and also other non-mobile drivers, so we need some more generic name. The renaming strategy could be exactly the same as described above

AppiumBy seems more relevant and as you said let's deprecate complete MobileBy stuffs.

src/main/java/io/appium/java_client/AppiumBy.java

src/main/java/io/appium/java_client/MobileBy.java

mykola-mokhnach · 2021-10-26T20:30:23Z

src/main/java/io/appium/java_client/AppiumBy.java

+    }
+
+    @Override public String toString() {
+        return String.format("By.%s: %s", locatorName, remoteParameters.value());


Maybe AppiumBy?

valfirst requested review from SrinivasanTarget and mykola-mokhnach October 21, 2021 08:17

valfirst force-pushed the unify-naming-of-factory-methods-in-mobile-by branch from bcac648 to 8e7d772 Compare October 21, 2021 08:19

mykola-mokhnach reviewed Oct 21, 2021

View reviewed changes

src/main/java/io/appium/java_client/MobileBy.java Outdated Show resolved Hide resolved

valfirst marked this pull request as draft October 22, 2021 12:23

valfirst force-pushed the unify-naming-of-factory-methods-in-mobile-by branch from 8e7d772 to 2cf96a7 Compare October 24, 2021 21:30

valfirst changed the title ~~refactor!: unify MobileBy methods naming and toString implementations~~ refactor: unify locator factories naming and toString implementations Oct 24, 2021

valfirst marked this pull request as ready for review October 24, 2021 21:34

valfirst requested a review from mykola-mokhnach October 24, 2021 21:34

SrinivasanTarget approved these changes Oct 25, 2021

View reviewed changes