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

Session details #201

Merged
merged 4 commits into from
Dec 19, 2017
Merged

Session details #201

merged 4 commits into from
Dec 19, 2017

Conversation

TikhomirovSergey
Copy link
Contributor

Change list

  • a new interface IHasSessionDetails
  • the IHasSessionDetails is implemented by AppiumDriver
  • tests

Types of changes

  • 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

I have started to add features which have been add to java clint since Jan'17

- IHasSessionDetails was added and implemented by AppiumDriver
- a new test SessionDetailTest.
- bug fixes.
Copy link
Contributor

@Astro03 Astro03 left a comment

Choose a reason for hiding this comment

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

just had some minor comments. ^_^ btw, are you still on slack? i'm sorta back.

@@ -425,7 +244,7 @@ public void Rotate(Dictionary<string, int> opts)

public void HideKeyboard() => AppiumCommandExecutionHelper.HideKeyboard(this, null, null);

/// <sumary>
/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to remove all the others, might as well remove this one too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary here because it is not declared by any interface or super class

get
{
var platform = GetSessionDetail("platformName");
if (platform == null || string.IsNullOrEmpty(Convert.ToString(platform)))
Copy link
Contributor

Choose a reason for hiding this comment

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

var platform = (String)GetSessionDetails("platformName");
if (string.isnullorempty(platform)) { platform = (String)GetSessionDetail("platform");}
return platform;

OR

i like this way
var platform = (String)GetSessionDetail("platformName");
return (string.IsNullOrEmpty(platform)) ? (String)GetSessionDetail("platform") : platform;

(or use "as" instead of cast)

{
get
{
var automation = GetSessionDetail("automationName");
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, why not just cast to a string? then return the casted string. (I haven't looked at the underlying object, are you doing this because the object is not a string type typically?)

also, you can use the "automationName" const in the MobileCapabilityType file.

}

public bool IsBrowser => GetSessionDetail("browserName") != null
&& Context.IndexOf("NATIVE_APP", StringComparison.OrdinalIgnoreCase) < 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

for both "browerName" and "NATIVE_APP" can use their "static readonly" in their respective files.

@Astro03
Copy link
Contributor

Astro03 commented Dec 18, 2017

@TikhomirovSergey are you online? want to get on slack?

- code style issues were fixed out
@TikhomirovSergey
Copy link
Contributor Author

@Astro03 There is the change. Could you take a look at this.

PS: there is some tech issue. I can't log in to slack cuz I can't receive sms with a code for some reason

@mykola-mokhnach
Copy link

@TikhomirovSergey have you tried to contact Slack's customer support? Perhaps there are other ways of getting the code (like Google Authenticator). There is one important topic I'd like to discuss about W3C stuff support by java client

Copy link
Contributor

@Astro03 Astro03 left a comment

Choose a reason for hiding this comment

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

lgtm. just had that one comment.

public string AutomationName => GetSessionDetail(MobileCapabilityType.AutomationName) as string;

public bool IsBrowser => GetSessionDetail(MobileCapabilityType.BrowserName) != null
&& Context.IndexOf("NATIVE_APP", StringComparison.OrdinalIgnoreCase) < 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the native_app const as well?

@Astro03
Copy link
Contributor

Astro03 commented Dec 18, 2017

btw, my personal preference is to put {} around all blocks, even if it's a one line if stmt or return.

- code style issues were fixed out
@TikhomirovSergey TikhomirovSergey merged commit 8a74cf5 into appium:master Dec 19, 2017
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.

None yet

3 participants