Download multiple packages trough the CLI and refactor

[fernandocollova]

Hi,

Following up on #58, here is some code that, among other changes, enables the download of multiple packages trough the CLI.

This is not the final PR I wish to merge, but I'd like to get some early feedback before I focus on fixing the tests and the docs.

There are a number of things to note about this code:

• Now that you can download more than one package trough the CLI, the idea of specifying a single filename as the output, as far as I can tell, won't work any more. That's why you now need to provide the path to a folder as an output, which will we be created if it doesn't exist, an all downloaded files will be placed there. The default is still Path.cwd() / Downloads and the tagging continues to be the same.
• get_cmd_args can't be passed any more params. I think calling the underlaying main, which has the same signature, is a more convenient solution, and leaves get_cmd_args to do the sole task of parsing cli args. All the defaults have been moved to the main also.
• download.py is gone. I'm happy to bring it back, but it seemed redundant now that you can call the downloader as an executable module with python3 -m playstoredownloader.cli
• Files are now downloaded using a single api object, meaning there is only one auth for downloading N packages. This is simply the change I thought was the best to make, as discussed in the Github issue, and I left it here as it is for review, because I wasn't sure if it needed to be rolled back. However, its trivial to change it the way it was before, so let me know if you want to try this or not.

Also, I did the async changes that I wanted here in case you are curious. As you can see, in the diff from the async to the regular one there are not a lot of changes, its just a matter of changing some calls to requests to aiohttp

Also, regarding the web part, I was looking into adding it to the CLI, but I noticed two things worth discussing:

• There is a reference to some private_credentials.json which I don't know how you would like to handle
• I'm not sure how to structure the CLI params, as using, for example a --web switch while passing packages to download doesn't make much sense. I'm inclined to just document it in the help and be done with it, saying that --web ignores packages to download, but I'd like your opinion.

That's all, hope you find this changes useful, and I look forward for some feedback about them :)
Regards, Fernando

Type of change

• Bug fix (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)
• Other (e.g., refactoring, documentation etc.)

Additional context

Checklist

• I have read the contributing guidelines
• I have performed a self-review of my own code (added comments, corrected misspellings etc.)
• My code follows the style guidelines of this project
• I have made corresponding changes to the documentation (if appropriate)
• I have added tests that prove my fix is effective or that my feature works
• New and existing tests pass locally with my changes

[ClaudiuGeorgiu]

First of all, thank you @fernandocollova for the contribution! I will give you some feedback, just my own opinions, so you may disagree 😉.

The commits are a bit messy: 2 WIP and 3 initial commits, this way is really hard to review the actual changes. You provided an extensive explanation of the changes in the pull request, so I guess you're excused this time, but in the future please consider squashing the commits when the pull request is ready for review, otherwise it will be hard to provide precise feedback.

• Now that you can download more than one package trough the CLI, the idea of specifying a single filename as the output, as far as I can tell, won't work any more. That's why you now need to provide the path to a folder as an output, which will we be created if it doesn't exist, an all downloaded files will be placed there. The default is still Path.cwd() / Downloads and the tagging continues to be the same.

Good point. Maybe we can still keep the single filename in case of a single download (as now) and raise an error if -o is used with multiple package names. In that case we would have to introduce a new parameter to specify the directory (-d?). I'm still undecided on this, what do you think?

• get_cmd_args can't be passed any more params. I think calling the underlaying main, which has the same signature, is a more convenient solution, and leaves get_cmd_args to do the sole task of parsing cli args. All the defaults have been moved to the main also.

The paramters were passed that way only because it was easier to unit test that code. If you modify the tests accordingly to keep the test coverage, I'm fine with the change.

• download.py is gone. I'm happy to bring it back, but it seemed redundant now that you can call the downloader as an executable module with python3 -m playstoredownloader.cli

Ok, I agree, with this change it's redundant.

• Files are now downloaded using a single api object, meaning there is only one auth for downloading N packages.

Single auth for N packages is perfectly fine as long as N is not a big number. With N < 10 there shouldn't be any problems (to be tested though).

• There is a reference to some private_credentials.json which I don't know how you would like to handle

As the name suggests, those are the private credentials that I use during development and are excluded from git. If that file exists, then it is used instead of credentials.json, and I would like to continue using it that way.

• I'm not sure how to structure the CLI params, as using, for example a --web switch while passing packages to download doesn't make much sense. I'm inclined to just document it in the help and be done with it, saying that --web ignores packages to download, but I'd like your opinion.

For the web part, you don't need to specify the packages to download from the CLI. I would use a different subpackage for this: python3 -m playstoredownloader.web that takes no paramter. The web interface is only a "bonus" feature anyway.

I will provide more feedback as a code review, but I haven't tried to run the code yet.

[fernandocollova]

Hey, just passing by to say that I haven't forgot about this 😅 but I had a lot of work this week. I'll try to work on this tomorrow or some time next week

[fernandocollova]

Hey, I finally have some time for this, and below I'll answer your points:

The commits are a bit messy: 2 WIP and 3 initial commits, this way is really hard to review the actual changes. You provided an extensive explanation of the changes in the pull request, so I guess you're excused this time, but in the future please consider squashing the commits when the pull request is ready for review, otherwise it will be hard to provide precise feedback.

Yeh, I guess fair enough. I'm not big on having pretty commit streams, but I can do better for next time.

Good point. Maybe we can still keep the single filename in case of a single download (as now) and raise an error if -o is used with multiple package names. In that case we would have to introduce a new parameter to specify the directory (-d?). I'm still undecided on this, what do you think?

Regarding the folder name, I much rather leave the single --out param, and then check if what you passed is a folder or a file. Then if you user multiple package names with a file, I raise an exception. Regarding that, if we are bringing that feature back, should the filename provided --out be tagged with the --tag? Also, should that feature be brought back? I see a maybe in your statement, but I don't know how strong that maybe is 😅. I personally found it a little bit messy to implement, as you need to deconstruct the filename to get the name (in case you want to tag it) and then sort of inject it into de download output for it to be used instead of the package name. I'm not against doing it however, just not jumping about the excitement.

The paramters were passed that way only because it was easier to unit test that code. If you modify the tests accordingly to keep the test coverage, I'm fine with the change.

Sure, as I said, I haven't had the time to go over the tests yet, but I will fix every test that needs fixing.

As the name suggests, those are the private credentials that I use during development and are excluded from git. If that file exists, then it is used instead of credentials.json, and I would like to continue using it that way.

If that's what you want, I guess I'll leave it as it is, but can I at least implement that behaibour in the cli tool too? Just for consistency.

For the web part, you don't need to specify the packages to download from the CLI. I would use a different subpackage for this: python3 -m playstoredownloader.web that takes no paramter. The web interface is only a "bonus" feature anyway.

Yes, that I understand, but you might want to provide the download output folder for example. Or tagging. Re thinking about this, maybe I can do something class based for argparse, to share many base arguments between the two modules, and then add some specific ones on the cli case.

That's all, thanks for your comments, and I'll probably start working on the tests this week while we finish resolving the rest of the discussed features.

Regards, Fernando

[ClaudiuGeorgiu]

Regarding the folder name, I much rather leave the single --out param, and then check if what you passed is a folder or a file.

Let's keep it simple and do like you suggested in the first message (provide the path to a folder as an output, which will we be created if it doesn't exist, an all downloaded files will be placed there), but instead of calling the parameter --out let's call it -d/--dir: no matter how many package names are specified, only the destination directory can be specified from command line (so the old --out won't be an option anymore).

but can I at least implement that behaibour in the cli tool too? Just for consistency.

Yes, sure, good idea.

Yes, that I understand, but you might want to provide the download output folder for example. Or tagging. Re thinking about this, maybe I can do something class based for argparse, to share many base arguments between the two modules, and then add some specific ones on the cli case.

I guess it could be a nice improvement 👍.

[fernandocollova]

Hello again,

So, I was finally able to find some time to work on this. I think I addressed all of the points we talked about, but I'm having some problems with the tests. More precisely, there are some category related tests that are failing, that I'm not sure why they are failing, and I'm not sure they are related to my changes at all. Are those working on your end?

Also, please notice that I had to change the mock count on test_download_response_error and test_download_response_error_2 because I think those mock counts assume the account running the tests doesn't own the downloaded app, and I think I do, so they weren't passing. But again, that's my vague understanding of the problem, so please let me know if that's not the case.

And, finally, I took the liberty the arg you asked me to rename to -d/--dir to out_dir as dir is a python command, and I wanted to be consistent with the arg name across cli and app.

Regards, Fernando

[ClaudiuGeorgiu]

Hi @fernandocollova, thanks for the updates! I will need some time to review everything, and since now the command line arguments are a little bit different I will also need to create a new release and make sure the README and the docker image are updated. I'll let you know as soon as this PR will be merged, thanks again!

[fernandocollova]

No problem! Also, as a follow up on the tests thing, I did the obvious today, and ran the tests from the master branch of this repo, and I can report that some of them are failing when I run them with my account, and three of them are the categories ones I talked about. The other two I think were failing on my end too, and i fixed them, but I don't remember for sure.

Anyway, this is the list of tests failing for me:

FAILED test/test_playstore_api.py::TestApi::test_get_base_categories - KeyError: 'cat'
FAILED test/test_playstore_api.py::TestApi::test_list_app_by_valid_category - TypeError: argument of type 'NoneType' is not iterable
FAILED test/test_playstore_api.py::TestApi::test_list_app_by_valid_category_and_subcategory - AssertionError: assert 7 <= 5
FAILED test/test_playstore_api.py::TestApi::test_download_corrupted_obb - assert True is False
FAILED test/test_playstore_api.py::TestApi::test_download_response_error_2 - assert True is False

Regards, Fernando

[ClaudiuGeorgiu]

Hi @fernandocollova, finally I had some time to look at this. I confirm that some tests were failing on my end too on the master branch, but everything should be fixed now. I also made some changes to styling and documentation, but everything seems to work great 👍. I still need to update a few files and then I will push everything to master, sorry for the delay.