Simplified the StartsActivity by reducing the number of parameters through POJO class by email2vimalraj · Pull Request #579 · appium/java-client

email2vimalraj · Feb 22, 2017

Simplified the StartsActivity by reducing the number of parameters through POJO class

Change list

Currently the number of startActivity methods are unnecessary which can be solved by one single method with single parameter. So introduced the simple POJO class which in future can be expanded with any number of parameters.

Types of changes

What types of changes are you proposing/introducing to Java client?

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

The sample usage of new implementation is as follows:

Activity activity = new Activity();
activity.setAppPackage("com.foo");
activity.setAppActivity(".bar");
driver.startActivity(activity);

Simplified the StartsActivity by reducing the number of parameters through the POJO class.

email2vimalraj · Feb 22, 2017

Could someone please explain why the CI build has failed?
I'm unable to run the build in my machine. Getting compilation error.
Please have a look at the exception that I'm getting: https://gist.github.com/email2vimalraj/171ecdaa966db76e047e0cc7e2942481

email2vimalraj · Feb 22, 2017

@SrinivasanTarget : Kindly help me what I'm supposed to fix in the build error.

saikrishna321 · Feb 22, 2017

@email2vimalraj you have styling issues. Please fix them

https://travis-ci.org/appium/java-client/builds/204062313#L1987

you can ignore the warnings

SrinivasanTarget · Feb 22, 2017

@email2vimalraj Hey buddy sorry just checked your note. please fix the errors to review.

TikhomirovSergey · Feb 22, 2017

src/main/java/io/appium/java_client/android/StartsActivity.java

+     *     }
+     * </pre>
     *
-     * @param appPackage      The package containing the activity. [Required]


@email2vimalraj I'm against this change as it is. Please mark these methods Deprecated and with the description to use new ones

TikhomirovSergey · Feb 22, 2017

src/main/java/io/appium/java_client/android/Activity.java

+/**
+ * This is a simple POJO class to support the {@link StartsActivity}.
+ */
+public class Activity {


I this this POJO should have 2 mandatory parameteres: package and activity. Please add the constructor with these parameters.

SrinivasanTarget · Feb 22, 2017

src/main/java/io/appium/java_client/android/Activity.java

+     *
+     * @param appPackage The app package value.
+     */
+    public void setAppPackage(String appPackage) {


I like the idea behind this but have few things to be discussed here,

Is there a way to remove getters & setters? May be we need to come up with either custom annotation or something like https://github.com/rzwitserloot/lombok. May be not necessarily in this PR but subjective to discussion if there is any scope of improvement.

Read the user review who was using the lombok for more than a year: http://stackoverflow.com/a/12807937/1505987

I would recommend to stay away from lombok, even though I never used it personally. My justification would be obviously a javadoc and everytime we will have to generate the code.

I also don't recommend lombock. But i liked their way of using annotations.

email2vimalraj · Feb 22, 2017

The CI is still pending from long time. Could someone look into it?

SrinivasanTarget · Feb 22, 2017

The CI is still pending from long time. Could someone look into it?

Have restarted travisCI job lets see. Anyways we need to wait for Codacy to complete.

email2vimalraj · Feb 22, 2017

Have restarted travisCI job lets see. Anyways we need to wait for Codacy to complete.

Seems some issue with Travis, it keeps the job in queue, but not running it up.

SrinivasanTarget · Feb 22, 2017

Seems some issue with Travis, it keeps the job in queue, but not running it up.

Yes checking

email2vimalraj · Feb 23, 2017

Any updates on CI?

email2vimalraj · Feb 23, 2017

@SrinivasanTarget @TikhomirovSergey : Can any one of you please help on the Travis issue? Seems the travis doesn't like to run my PR :(

SrinivasanTarget · Feb 23, 2017

@email2vimalraj I'll check with @imurchie and see if we could do something.

email2vimalraj · Feb 23, 2017

Seems Travis finally ran my PR and this time build passed. :)

https://travis-ci.org/appium/java-client/builds/204446236#L1990

SrinivasanTarget

Otherwise LGTM

SrinivasanTarget · Feb 23, 2017

docs/The-starting-of-an-Android-app.md


-StartsActivity startsActivity.startActivity("your.package.name", ".ActivityName");
+Activity activity = new Activity();
+activity.setAppPackage("your.package.name");


@email2vimalraj Can you update here too.

@email2vimalraj ping

email2vimalraj · Feb 24, 2017

@SrinivasanTarget : Fixed, kindly have a look

TikhomirovSergey · Feb 25, 2017

src/main/java/io/appium/java_client/android/Activity.java

+     * @param appPackage The value for the app package.
+     * @param appActivity The value for the app activity.
+     */
+    public Activity(String appPackage, String appActivity) {


@email2vimalraj Could you provide these checkings:

import static com.google.common.base.Preconditions.checkArgument; import static org.apache.commons.lang3.StringUtils.isBlank; ... public Activity(String appPackage, String appActivity) { checkArgument(!isBlank(appPackage), "App package should be defined as not empty or null string"); checkArgument(!isBlank(appActivity), "App activity should be defined as not empty or null string"); ... }

Good catch 👍

TikhomirovSergey · Feb 25, 2017

src/main/java/io/appium/java_client/android/Activity.java

+     *
+     * @param appActivity The app activity value.
+     */
+    public void setAppActivity(String appActivity) {


I'm not sure that it is necessary to keep this method. Maybe it is more senseful to keep only getter for activity and make this field final. @SrinivasanTarget What is your opinion?

TikhomirovSergey · Feb 25, 2017

src/main/java/io/appium/java_client/android/Activity.java

+     *
+     * @param appPackage The app package value.
+     */
+    public void setAppPackage(String appPackage) {


I'm not sure that it is necessary to keep this method. Maybe it is more senseful to keep only getter for package and make this field final. @SrinivasanTarget What is your opinion?

+1 - Making this final would be a better option 👍

TikhomirovSergey · Feb 25, 2017

@email2vimalraj I have finished the reviewing. It is almost ok. @email2vimalraj Could you read comments and make some improvenents?

ping @SrinivasanTarget

…Also made the mandatory parameters final and removed setters.

email2vimalraj · Feb 25, 2017

@TikhomirovSergey and @SrinivasanTarget : Done the changes as recommended

TikhomirovSergey · Feb 25, 2017

@email2vimalraj I have approved these changes. @SrinivasanTarget ?

SrinivasanTarget · Feb 25, 2017

@email2vimalraj It's a merge away. Waiting for Codacy to be happy.

SrinivasanTarget · Feb 25, 2017

Thanks @email2vimalraj 👍

Addition to the #579

4140e33

Simplified the StartsActivity by reducing the number of parameters through the POJO class.

SrinivasanTarget self-requested a review February 22, 2017 03:59

SrinivasanTarget added refactor Android labels Feb 22, 2017

TikhomirovSergey suggested changes Feb 22, 2017

View reviewed changes

Addressed review comments

9288d33

SrinivasanTarget reviewed Feb 22, 2017

View reviewed changes

Added javadoc for the constructor.

bf96700

SrinivasanTarget requested changes Feb 23, 2017

View reviewed changes

Fixed usage instructions

869dcde

TikhomirovSergey suggested changes Feb 25, 2017

View reviewed changes

TikhomirovSergey added this to the 5.0.0 milestone Feb 25, 2017

TikhomirovSergey suggested changes Feb 25, 2017

View reviewed changes

Added a check to prevent Activity parameters not empty and not null. …

42c64d5

…Also made the mandatory parameters final and removed setters.

TikhomirovSergey approved these changes Feb 25, 2017

View reviewed changes

SrinivasanTarget approved these changes Feb 25, 2017

View reviewed changes

SrinivasanTarget merged commit 72b9e6a into appium:master Feb 25, 2017

TikhomirovSergey added a commit that referenced this pull request Feb 28, 2017

Merge pull request #585 from TikhomirovSergey/master

b9ef941

Addition to the #579

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

email2vimalraj commented Feb 22, 2017

Change list

Types of changes

Details

Uh oh!

email2vimalraj commented Feb 22, 2017

Uh oh!

email2vimalraj commented Feb 22, 2017

Uh oh!

saikrishna321 commented Feb 22, 2017

Uh oh!

SrinivasanTarget commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SrinivasanTarget Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

email2vimalraj commented Feb 22, 2017

Uh oh!

SrinivasanTarget commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

email2vimalraj commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SrinivasanTarget commented Feb 22, 2017

Uh oh!

email2vimalraj commented Feb 23, 2017

Uh oh!

email2vimalraj commented Feb 23, 2017

Uh oh!

SrinivasanTarget commented Feb 23, 2017

Uh oh!

email2vimalraj commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

email2vimalraj commented Feb 24, 2017

Uh oh!

TikhomirovSergey Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Feb 25, 2017

Uh oh!

email2vimalraj commented Feb 25, 2017

Uh oh!

TikhomirovSergey commented Feb 25, 2017

Uh oh!

SrinivasanTarget commented Feb 25, 2017

Uh oh!

SrinivasanTarget commented Feb 25, 2017

Uh oh!

Reviewers

Assignees

SrinivasanTarget Feb 22, 2017 •

edited

Loading

SrinivasanTarget commented Feb 22, 2017 •

edited

Loading

email2vimalraj commented Feb 22, 2017 •

edited

Loading

email2vimalraj commented Feb 23, 2017 •

edited

Loading

TikhomirovSergey Feb 25, 2017 •

edited

Loading