Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

download returns inconsistent types (Paths or strs) depending on in_region #595

Open
itcarroll opened this issue Jun 10, 2024 · 5 comments
Labels
breaking A change that will require some or all users to adapt good first issue Good for newcomers

Comments

@itcarroll
Copy link
Collaborator

itcarroll commented Jun 10, 2024

The download docs say that the return value shall be a list of strings. When running in_region however, the result is a list of pathlib.PosixPath objects.

import earthaccess
import pathlib

assert earthaccess.__version__ == "0.9.0"
assert earthaccess.__store__.in_region

local = pathlib.Path("data").mkdir(exist_ok=True)
results = earthaccess.search_data(short_name="PACE_OCI_L2_BGC_NRT", count=1)
paths = earthaccess.download(results, local_path=local)
assert isinstance(paths[0], str)

The last assert raises an exception.

@mfisher87
Copy link
Collaborator

Nice catch! Why isn't our type checker catching this though?

) -> List[str]:

I love pathlib though, should we update download() to always return Paths? :)

@itcarroll
Copy link
Collaborator Author

If you want to be fancy, you could return the same type given to you in the local_path argument.

@andypbarrett
Copy link
Collaborator

I like the idea of returning a list of Path objects. I think this would avoid cross platform issues.

@meteodave
Copy link

I would like to upvote this for earthaccess.download() to be consistently returning an object of the same type, whether it be Posix or string (probably should be Posix for cross platform compatibility although several folks may have developed code already assuming the string list). In my script, I evaluate the earthaccess.download() returned object within my script to check whether I am in region or not and then apply list comprehension line to convert the Posix object to a string list since another function is expecting a string list.

files = earthaccess.download(granules, path)
if earthaccess.__store__.in_region == True:  files =  [str(Path(f)) for f in files]       

@mfisher87 mfisher87 changed the title download returns Path objects rather than strings when in_region download returns inconsistent types (Paths or strs) depending on in_region Sep 14, 2024
@mfisher87 mfisher87 added breaking A change that will require some or all users to adapt good first issue Good for newcomers labels Sep 14, 2024
@mfisher87
Copy link
Collaborator

although several folks may have developed code already assuming the string list

Excellent point. Let's please keep in mind as we address this that we also have work in progress to establish a compatibility policy: https://github.com/nsidc/earthaccess/pull/763/files We should include a prominent notification of breaking change and a migration guide with code examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that will require some or all users to adapt good first issue Good for newcomers
Projects
Status: 🆕 New
Development

No branches or pull requests

4 participants