driver.push_file(destination_path, source_path) feature by JavonDavis · Pull Request #270 · appium/python-client

JavonDavis · 2018-11-16T07:58:52Z

This PR aims to simplify pushing and retrieving files from the simulator and emulator. It provides an optional parameter source_path to the existing push_file function and does the base64 conversion on that file for users.

Fixes #269

To test:
/Users/javon/Projects/python-client/test/functional/android/push_file_tests.py
/Users/javon/Projects/python-client/test/functional/ios/push_file_tests.py

jsf-clabot · 2018-11-16T07:58:53Z

All committers have signed the CLA.

mykola-mokhnach · 2018-11-16T08:15:08Z

Did you test the PR for both Python 2 and Python3?
I remember there was an issue while base-64 encoding a string there. See #267 for more details.

mykola-mokhnach · 2018-11-16T08:19:57Z

+        Specify either `base64data` or `source_path`, if both specified default to `source_path`
        :Args:
-         - path - the path on the device
+         - destination_path - the path on the device


I'd rather add more advanced docstrinmg here, since several formatting options are available there. Check https://github.com/appium/java-client/blob/f527be18735f8a60b4f6244f03a1b9954c23a60f/src/main/java/io/appium/java_client/InteractsWithFiles.java#L44

and

https://github.com/appium/appium-xcuitest-driver/blob/31059c4169516098d2b64a328197a53000ecd360/lib/commands/file-movement.js#L186

for more details

Sure I update the formatting to the reStructured text format, let me know if that one's good. Originally I was just going along with the format I saw in the rest of the file to stay consistent but now I'm noticing some other functions use this format as well.

mykola-mokhnach · 2018-11-16T08:20:27Z

+        if source_path is None and base64data is None:
+            raise InvalidArgumentException('Must either present base64 data or a source path on local system')
+
+        if source_path is not None:


should we check if the file exists first?

i think we should, either that or catch the exception and provide something meaningful as a user facing error

The error that it threw was in this is FileNotFoundError: [Errno 2] No such file or directory: '{filename}', I'm not sure the message I'm presenting now is much different source_path {} could not be found. Are you sure the file exists? Is there a better way to do this or this should be good enough?

jlipps

Thanks so much for this, @JavonDavis, it's a great contribution! I made some additional comments that should hopefully be pretty easy to resolve.

jlipps · 2018-11-16T19:10:38Z

+        if source_path is None and base64data is None:
+            raise InvalidArgumentException('Must either present base64 data or a source path on local system')
+
+        if source_path is not None:


i think we should, either that or catch the exception and provide something meaningful as a user facing error

mvgiacomello · 2018-11-17T20:39:24Z

+        if source_path is None and base64data is None:
+            raise InvalidArgumentException('Must either present base64 data or a source path on local system')
+
+        if source_path is not None:


You could use if bool(source_path):

Not sure there's a real benefit to converting to a boolean for the check?

I like the current state of the code (with the slightly more informative error message). Thanks.

mvgiacomello · 2018-11-17T20:44:42Z

+        'automationName': 'UIAutomator2'
    }

    return desired_caps


Do we use newline at end of file as per PEP8?

It passes the pep8 check locally and I didn't make any change at the end of this file just really the addition of UIAutomator2

python -m autopep8 -r --global-config .config-pep8 -d . runs on CI. I've relaxed the check for max-line-length, only. Is it enough for pep8?
(If I missed something, I'd like to add the check on CI)

@KazuCocoa I think the relaxed 120 max line length is pretty good

* changed docstring format * Catch error thrown if file not present and present user with a better message

JavonDavis · 2018-11-19T00:19:08Z

Did you test the PR for both Python 2 and Python3?
I remember there was an issue while base-64 encoding a string there. See #267 for more details.

Hey @mykola-mokhnach thanks for the review and I actually ran into that issue while I was working on this, encoding with base64.b64encode(data).decode('utf-8') seemed to work well across both the Python versions I tested with (2.7 and 3.7)

JavonDavis · 2018-11-19T00:25:48Z

Thanks so much for this, @JavonDavis, it's a great contribution! I made some additional comments that should hopefully be pretty easy to resolve.

@jlipps Sure no problem! Thanks for the review 👍 I've made some changes based on the comments

mykola-mokhnach · 2018-11-19T06:06:16Z

            'path': path,
        }
-        return self.execute(Command.PULL_FILE, data)['value']
+        return base64.b64decode(self.execute(Command.PULL_FILE, data)['value'])


I would say we should not do this, since it is going to break the backward compatibility.

You're right I overlooked this, it's removed

mykola-mokhnach · 2018-11-19T06:08:15Z

-         - path - the path on the device
-         - base64data - data, encoded as Base64, to be written to the file
+        Specify either `base64data` or `source_path`, if both specified default to `source_path`
+        :param destination_path: the path on the device/simulator


Please add more detailed description of this argument

Is the new description a little better?

…cstring description for `destination_path`

jlipps · 2018-11-19T17:32:38Z

+        if source_path is None and base64data is None:
+            raise InvalidArgumentException('Must either present base64 data or a source path on local system')
+
+        if source_path is not None:


I like the current state of the code (with the slightly more informative error message). Thanks.

KazuCocoa · 2018-11-21T06:16:06Z

Thanks! Will merge this.

JavonDavis added 3 commits November 16, 2018 02:50

used base64 library for conversion

e70f69b

remove unnecessary library use

f22ee8d

changed text in test file

429b267

mykola-mokhnach requested a review from KazuCocoa November 16, 2018 08:15

mykola-mokhnach reviewed Nov 16, 2018

View reviewed changes

Comment thread appium/webdriver/webdriver.py Outdated

mykola-mokhnach reviewed Nov 16, 2018

View reviewed changes

jlipps requested changes Nov 16, 2018

View reviewed changes

mvgiacomello reviewed Nov 17, 2018

View reviewed changes

JavonDavis added 2 commits November 18, 2018 17:04

* Using context when reading file

6600fed

* changed docstring format * Catch error thrown if file not present and present user with a better message

fixed incorrect file path in test

15da22a

mykola-mokhnach reviewed Nov 19, 2018

View reviewed changes

removed change in pul_file that broke backwards compat and updated do…

8d2e242

…cstring description for `destination_path`

jlipps approved these changes Nov 19, 2018

View reviewed changes

KazuCocoa approved these changes Nov 19, 2018

View reviewed changes

KazuCocoa merged commit b6795db into appium:master Nov 21, 2018

Uh oh!

Conversation

JavonDavis commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsf-clabot commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mykola-mokhnach commented Nov 16, 2018

Uh oh!

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!

JavonDavis Nov 19, 2018 • 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!

jlipps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

JavonDavis commented Nov 19, 2018

Uh oh!

JavonDavis commented Nov 19, 2018

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!

KazuCocoa commented Nov 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JavonDavis commented Nov 16, 2018 •

edited

Loading

jsf-clabot commented Nov 16, 2018 •

edited

Loading

JavonDavis Nov 19, 2018 •

edited

Loading