Added format to device_time as argument by ki4070ma · Pull Request #312 · appium/python-client

ki4070ma · 2019-01-08T04:06:00Z

Which option should I take? (Maybe No.2?)

Removed property decorator from device_time
- c2c5f98
- [Pros] Normal api style
- [Cons] Impact to current users
Added another api like device_time_format()
- [Pros] No impact to current users
- [Cons] Increased similar api (e.g. device_time, device_time_format())

mykola-mokhnach · 2019-01-08T06:56:53Z

How about the third option - we keep the property and add a new setter for datetime format?

ki4070ma · 2019-01-08T07:41:43Z

Thanks for your comment.
Do you mean that 'new setter' is for property?
If yes, setter for property can't be used in my understanding.

https://docs.python.org/3/library/functions.html?highlight=property#property

if no, could you tell more for details?

KazuCocoa · 2019-01-08T09:29:57Z

It means like below, I assume

    @property
    def device_time(self):
        if self._device_time_format:
            return self.execute(Command.POST_DEVICE_TIME, {'format': format})['value']
        else:
            return self.execute(Command.GET_DEVICE_TIME, {})['value']
            
    def set_device_time_format(self, format):
        self._device_time format = format

ki4070ma · 2019-01-08T10:29:58Z

@KazuCocoa @mykola-mokhnach
Thanks, I see. I didn't have such idea to have the instance member.
Your idea looks better, I'll follow it.

ki4070ma · 2019-01-08T12:45:17Z

appium/webdriver/webdriver.py

        self.command_executor._commands[Command.LOCATION_IN_VIEW] = \
            ('GET', '/session/$sessionId/element/$id/location_in_view')
        self.command_executor._commands[Command.GET_DEVICE_TIME] = \
-            ('GET', '/session/$sessionId/appium/device/system_time')


@KazuCocoa

Can I change to only 'POST'?
My motivation is just to avoid complicated naming around below codes.

python-client/appium/webdriver/webdriver.py

Lines 587 to 588 in a40a933

self.command_executor._commands[Command.GET_DEVICE_TIME] = \

('GET', '/session/$sessionId/appium/device/system_time')

e.g.

self.command_executor._commands[Command.GET_DEVICE_TIME_GET] = \ ('GET', '/session/$sessionId/appium/device/system_time') self.command_executor._commands[Command.GET_DEVICE_TIME_POST] = \ ('POST', '/session/$sessionId/appium/device/system_time')

Your idea will be different from other variable naming..

If GET behavior should be kept, I'll update so.
(I'm not sure the impact to current behavior by change from POST to GET.)

Thanks.

You should define another command like your e.g. as compatibility for released Appium servers.
Current released appium server cannot handle the post method. We observe users who use older Appium servers. Other libraries also use this library in their libs.

Thus, we would like to avoid breaking changes as possible.
(Instead, please leave a comment we can remove it in the future, for instance.)

I'd also keep the old GET endpoint. I was thinking on that and I think it is more logical to leave the existing property as as and add get_formatted_device_time(self, format) method, which is going to call the new POST. Also, in the documentation for this method it is necessary to mention it is only available since Appium 1.11

@KazuCocoa @mykola-mokhnach
Thanks a lot, you're right. I should have considered more.
I'll update along to your comments.

ki4070ma · 2019-01-09T07:34:29Z

appium/webdriver/webdriver.py

+        return self.execute(Command.GET_DEVICE_TIME_GET, {})['value']
+
+    def get_device_time(self, format=''):
+        """Returns the date and time from the device. (Only available since Appium 1.11.0)


TODO: Need to check next released Appium server version which includes appium/appium-base-driver#294

Like below. You can try out using current appium@beta.

[HTTP] --> POST /wd/hub/session/5cb5ebe6-099d-4bdf-b516-614dad889d3e/appium/device/system_time [HTTP] {} [debug] [W3C (5cb5ebe6)] Calling AppiumDriver.getDeviceTime() with args: [null,"5cb5ebe6-099d-4bdf-b516-614dad889d3e"] [AndroidDriver] Attempting to capture android device date and time [debug] [ADB] Running '/Users/kazuaki/Library/Android/sdk/platform-tools/adb -P 5037 -s emulator-5554 shell date +%Y-%m-%dT%T%z' [debug] [W3C (5cb5ebe6)] Responding to client with driver.getDeviceTime() result: "2019-01-09T18:26:40+09:00" [HTTP] <-- POST /wd/hub/session/5cb5ebe6-099d-4bdf-b516-614dad889d3e/appium/device/system_time 200 56 ms - 37 [HTTP] [HTTP] --> POST /wd/hub/session/5cb5ebe6-099d-4bdf-b516-614dad889d3e/appium/device/system_time [HTTP] {"format":"YYYY-MM-DD"} [debug] [W3C (5cb5ebe6)] Calling AppiumDriver.getDeviceTime() with args: ["YYYY-MM-DD","5cb5ebe6-099d-4bdf-b516-614dad889d3e"] [AndroidDriver] Attempting to capture android device date and time [debug] [ADB] Running '/Users/kazuaki/Library/Android/sdk/platform-tools/adb -P 5037 -s emulator-5554 shell date +%Y-%m-%dT%T%z' [debug] [W3C (5cb5ebe6)] Responding to client with driver.getDeviceTime() result: "2019-01-09" [HTTP] <-- POST /wd/hub/session/5cb5ebe6-099d-4bdf-b516-614dad889d3e/appium/device/system_time 200 36 ms - 22 [HTTP]

Thanks. Yes, I've checked behavior with my changes in my local.
My TODO means I'll need to update version on comment if next version is different from 1.11.0. (e.g. 1.10.1)

If my understanding is wrong, please let me know.

Thanks.

Gocha. The next is 1.11.0. Current number is correct

Thanks, I've removed 'DO NOT MERGE' from issue's title.

mykola-mokhnach · 2019-01-09T17:48:31Z

appium/webdriver/webdriver.py

-        return self.execute(Command.GET_DEVICE_TIME, {})['value']
+        return self.execute(Command.GET_DEVICE_TIME_GET, {})['value']
+
+    def get_device_time(self, format=''):


the default value should be None

mykola-mokhnach · 2019-01-09T17:49:03Z

appium/webdriver/webdriver.py

+        :Usage:
+            self.driver.get_device_time(self, "YYYY-MM-DD")
+        """
+        data = {'format': format} if format else {}


if format is None then device_time should be returned

mykola-mokhnach · 2019-01-10T06:44:54Z

appium/webdriver/webdriver.py

+        """
+        if format:
+            return self.execute(Command.GET_DEVICE_TIME_POST, {'format': format})['value']
+        else:


if format is None (https://docs.quantifiedcode.com/python-anti-patterns/readability/comparison_to_none.html)

also, else is redundant

if format is None: return self.device_time return self.execute(Command.GET_DEVICE_TIME_POST, {'format': format})['value']

is better than below?

if format: return self.execute(Command.GET_DEVICE_TIME_POST, {'format': format})['value'] return self.device_time

also, else is redundant

oh,,,thanks.

Done by 54815e8

The PR indicates the default argument is None. It is good to handle return value following the None then. None is better than "", since we can compare them as literal explicitly.

mykola-mokhnach · 2019-01-10T06:46:19Z

appium/webdriver/webdriver.py

+        :Args:
+         - format - The default format is `YYYY-MM-DDTHH:mm:ssZ`, which complies to ISO-8601
+           The set of format specifiers. Read https://momentjs.com/docs/ to get
+           the full list of supported datetime format specifiers.


please also add that if unset then the result of device_time is returned by default

Done by 771324b

mykola-mokhnach · 2019-01-10T18:08:03Z

I would also move the device-time related methods to a separate extension class, like in
#307

mykola-mokhnach · 2019-01-10T18:08:16Z

Although, this can be done in the other PR

mykola-mokhnach requested a review from KazuCocoa January 8, 2019 06:57

ki4070ma added 3 commits January 8, 2019 21:11

Added format to device_time as argument

7ebd12c

Added date format setter for device_time

a83b491

Add unittest for set_device_time_format

5e7e1ab

ki4070ma commented Jan 8, 2019

View reviewed changes

Replaced with get_device_time in order to get formatted time

228ac44

ki4070ma commented Jan 9, 2019

View reviewed changes

mykola-mokhnach reviewed Jan 9, 2019

View reviewed changes

Applied comments for get_device_time

5e0d041

ki4070ma changed the title ~~[DO NOT MERGE] Added format to device_time as argument~~ Added format to device_time as argument Jan 10, 2019

mykola-mokhnach reviewed Jan 10, 2019

View reviewed changes

ki4070ma added 3 commits January 10, 2019 20:59

Updated comment

771324b

Removed unnecessary codes

54815e8

Removed empty lines

4dad547

KazuCocoa approved these changes Jan 10, 2019

View reviewed changes

mykola-mokhnach approved these changes Jan 10, 2019

View reviewed changes

mykola-mokhnach merged commit fe4344f into appium:master Jan 10, 2019

ki4070ma mentioned this pull request Mar 16, 2019

Update system-time.yml appium/appium#12325

Merged

9 tasks

ki4070ma mentioned this pull request Jan 19, 2020

AttributeError: 'WebDriver' object has no attribute 'get_device_time' #483

Closed

	self.command_executor._commands[Command.GET_DEVICE_TIME] = \
	('GET', '/session/$sessionId/appium/device/system_time')

Conversation

ki4070ma commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mykola-mokhnach commented Jan 8, 2019

Uh oh!

ki4070ma commented Jan 8, 2019

Uh oh!

KazuCocoa commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ki4070ma commented Jan 8, 2019

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach commented Jan 10, 2019

Uh oh!

mykola-mokhnach commented Jan 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ki4070ma commented Jan 8, 2019 •

edited

Loading

KazuCocoa commented Jan 8, 2019 •

edited

Loading