[SYNPY-1802] Add OOP DownloadList class

[andrewelamb]

Problem:

Download list functionality was part of the main client class.

Solution:

Create a DownloadList OOP class with the same functionality
Deprecate the old methods

Testing:

Unit and Integration tests for the new class methods.

[linglp]

Hi @andrewelamb ! I focused on reading the tests for this round of review. I will take a look at the core functions later. Thanks for your hard work!

[linglp]

One main thing that I noticed is that some parts of the code would fail silently. For example:

num_files = DownloadList().add_files(
    files=[
        DownloadListItem(
            file_entity_id="syn24181472",
            version_number=1
        ),
        DownloadListItem(
            file_entity_id="syn24246013",
            version_number=100
        )
    ]
)

Unless users check num_files, there's no way of knowing if all the files have successfully been added to the download list or only some of them have been added.

Similarly, when you remove a file from download list, it is hard to know if all the files have successfully been removed or only some of them have been removed.

In addition, how can we download only a subset of items in the cart? My download cart has over a thousand files but I only want a subset of them to be downloaded. Currently, the only way to do this is to clear the cart, and then re-add specific files. But after clearing all the items in download cart, there's no way to restore them. It seems like there's an endpoint here that could let you query the download cart, but I didn't see it get implemented here. Please let me know if this is not part of the ticket.

[linglp]

This class is called DownloadListManifestRequest, but based on the docstring, it seems like this class is also the response class? I think the naming here is confusing.

[andrewelamb]

It's the name of the Endpoint: https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/download/DownloadListManifestRequest.html, I'm happy to change it to something else!

[linglp]

this is an interesting defensive check. The manifest content is entirely determined by the Synapse server by DownloadList.get_manifest_async. Why this manifest would have naming collision?

[andrewelamb]

Right, this is more of a guard against those being added in the future.

[andrewelamb]

One main thing that I noticed is that some parts of the code would fail silently. For example:

num_files = DownloadList().add_files(
    files=[
        DownloadListItem(
            file_entity_id="syn24181472",
            version_number=1
        ),
        DownloadListItem(
            file_entity_id="syn24246013",
            version_number=100
        )
    ]
)

Unless users check num_files, there's no way of knowing if all the files have successfully been added to the download list or only some of them have been added.

Similarly, when you remove a file from download list, it is hard to know if all the files have successfully been removed or only some of them have been removed.

In addition, how can we download only a subset of items in the cart? My download cart has over a thousand files but I only want a subset of them to be downloaded. Currently, the only way to do this is to clear the cart, and then re-add specific files. But after clearing all the items in download cart, there's no way to restore them. It seems like there's an endpoint here that could let you query the download cart, but I didn't see it get implemented here. Please let me know if this is not part of the ticket.

You are correct! This ticket was only for replicating the functionality in synapseclient/client.py that has been deprecated. I'll create a ticket to expand on this one.

[linglp]

I flag some issues with the docstring. Could you fix them? Also, I noticed a problem with get_manifest losing parameters in comparison with the old function. I am not sure if that's intentional

[andrewelamb]

Reverting back to draft, I found some issues that need to be adressed.