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

Add handlers for gsm, network, power and sms command #834

Merged
merged 5 commits into from
Feb 19, 2018
Merged

Add handlers for gsm, network, power and sms command #834

merged 5 commits into from
Feb 19, 2018

Conversation

SrinivasanTarget
Copy link
Member

Change list

Add handlers for gsm, network, power and sms command
https://github.com/appium/appium-base-driver/blob/master/lib/protocol/routes.js#L366-L375

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)

@mykola-mokhnach ping


private final String gsmcall;

GsmCallActions(String gsmcall) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this constructor is redundant. All enum elements have name() and ordinal() properties. That's pretty all you need. Same comment to other enums

@@ -176,4 +183,68 @@ public void toggleLocationServices() {
CommandExecutionHelper.execute(this, toggleLocationServicesCommand());
}

/**
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Feb 17, 2018

Choose a reason for hiding this comment

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

I'd rather put these methods into separate interface named as SupportsSpecialEmulatorCommands

*
* @return a key-value pair. The key is the command name. The value is a {@link Map} command arguments.
*/
public static Map.Entry<String, Map<String, ?>> powerACCommand(
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Feb 17, 2018

Choose a reason for hiding this comment

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

all these methods have one extra space between the return type and the method name

/**
* Emulate power state change on the connected emulator.
*
* @param powerACState One of available Power AC state.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather put a direct link to the corresponding enum here

@appium appium deleted a comment Feb 17, 2018
@SrinivasanTarget
Copy link
Member Author

SrinivasanTarget commented Feb 18, 2018

It looks like we need to add at least one assert to each test to make Codacy happy

Yeah but i'm not sure what to assert here? Do you have any better suggestion? @mykola-mokhnach

@mykola-mokhnach
Copy link
Contributor

try {
  smth();
} catch (Exception e) {
  fail(e)
}

try maybe this workaround

try {
driver.sendSMS("11111111", "call");
} catch (Exception e) {
assertFalse( "method works only in emulators", true);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use fail call instead

@appium appium deleted a comment Feb 18, 2018
* @param phoneNumber The phone number of the caller.
* @param gsmCallActions One of available {@link GsmCallActions} values.
*/
default void setGsmCall(String phoneNumber, GsmCallActions gsmCallActions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

makeGsmCall would be a better name

* @param gsmSignalStrength One of available {@link GsmSignalStrength} values.
*/
default void setGsmSignalStrength(GsmSignalStrength gsmSignalStrength) {
CommandExecutionHelper.execute( this, gsmSignalStrengthCommand(gsmSignalStrength));
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space before this

@SrinivasanTarget SrinivasanTarget merged commit 5ac71ab into appium:master Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants