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

email2vimalraj · 2017-02-22T03:50:05Z

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:

Activityactivity = newActivity();
activity.setAppPackage("com.foo");
activity.setAppActivity(".bar");
driver.startActivity(activity);

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

email2vimalraj · 2017-02-22T04:09:39Z

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 · 2017-02-22T04:33:37Z

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

saikrishna321 · 2017-02-22T05:06:26Z

@email2vimalraj you have styling issues. Please fix them

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

you can ignore the warnings

SrinivasanTarget · 2017-02-22T07:29:48Z

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

TikhomirovSergey · 2017-02-22T08:00:13Z

+ * }
+ * </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 · 2017-02-22T08:03:42Z

+/**
+ * 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 · 2017-02-22T17:24:20Z

+ *
+ * @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 · 2017-02-22T17:58:08Z

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

SrinivasanTarget · 2017-02-22T18:00:55Z

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 · 2017-02-22T18:13:06Z

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 · 2017-02-22T18:17:07Z

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

Yes checking

email2vimalraj · 2017-02-23T02:05:22Z

Any updates on CI?

email2vimalraj · 2017-02-23T12:45:13Z

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

SrinivasanTarget · 2017-02-23T14:03:37Z

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

email2vimalraj · 2017-02-23T14:05:29Z

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 · 2017-02-23T17:22:21Z


-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 · 2017-02-24T18:08:30Z

@SrinivasanTarget : Fixed, kindly have a look

TikhomirovSergey · 2017-02-25T13:39:59Z

+ * @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:
importstaticcom.google.common.base.Preconditions.checkArgument; importstaticorg.apache.commons.lang3.StringUtils.isBlank; ... publicActivity(StringappPackage, StringappActivity) { 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 · 2017-02-25T13:42:42Z

+ *
+ * @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 · 2017-02-25T13:43:38Z

+ *
+ * @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 · 2017-02-25T13:45:53Z

@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 · 2017-02-25T14:17:43Z

@TikhomirovSergey and @SrinivasanTarget : Done the changes as recommended

TikhomirovSergey · 2017-02-25T14:34:42Z

@email2vimalraj I have approved these changes. @SrinivasanTarget ?

SrinivasanTarget · 2017-02-25T14:51:20Z

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

SrinivasanTarget · 2017-02-25T14:52:20Z

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:masterFeb 25, 2017

TikhomirovSergey added a commit that referenced this pull request Feb 28, 2017
Merge pull request#585from TikhomirovSergey/master
b9ef941
Addition to the #579

Uh oh!

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!

SrinivasanTargetFeb 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!

TikhomirovSergeyFeb 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

SrinivasanTargetFeb 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

TikhomirovSergeyFeb 25, 2017•
edited
Loading