MAST Observations get_urls#11
Conversation
snbianco
force-pushed
the
mast-beta-release
branch
from
6413c31 to
b317f74
Compare
snbianco
left a comment
snbianco
left a comment
There was a problem hiding this comment.
Some initial review! I think some things will have to be reworked, so I'll do a more detailed review later on.
|
|
||
| return manifest | ||
|
|
||
| def get_urls(self, products, *, mrp_only=False, cloud_only=False, include_bucket=True, |
There was a problem hiding this comment.
We should call this function something more specific. Maybe get_download_urls or get_product_urls? @scfleming do you have any thoughts on this?
There was a problem hiding this comment.
Happy to change to whatever you/@scfleming think is best!
There was a problem hiding this comment.
Since these are URLs for products/files, I would suggest get_file_urls or get_product_urls, and out of the two I think "product" is more descriptive than "file" (which could be something other than a data product).
snbianco
force-pushed
the
mast-beta-release
branch
from
68e67e8 to
77cf87d
Compare
TAP URL constant, add comments Updating docstrings, don't set `available_collections` in method
| assert uri_list[0] == data_uri | ||
|
|
||
|
|
||
| def test_observations_get_product_urls(monkeypatch): |
There was a problem hiding this comment.
Could you break up this test a bit? Maybe so that you don't have to toggle the cloud as much.
| products = self.filter_products(products, mrp_only=mrp_only, **filters) | ||
|
|
||
| # remove duplicate products | ||
| products = utils.remove_duplicate_products(products, 'dataURI') |
There was a problem hiding this comment.
Can you add 1115-1120 to the helper function since it's the same in download_products? Also, if a list if raw URIs is passed that can't be filtered, the filter_products call should be skipped.
There was a problem hiding this comment.
download_products always applies filtering (and doesn't accept raw URIs), but get_product_urls intentionally skips filtering when the input is a list of raw URIs. Since download_products should always have an empty URI_list, we can move all of it into the helper _build_products_list (with the conditional of if uri_list is None), and it will always apply the filtering for download_products and only when required for get_product_urls
There was a problem hiding this comment.
Unfortunately, this is not actually possible. Moving the filtering logic leads to download_products failures which I imagine are related to an issue in URI resolution
3 participants
Implementing MAST Observation method that gets the list of URLs for a table product but doesn't download them. Includes tests for get_urls.