Skip to content

Conversation

@abradley60
Copy link
Collaborator

Copy link
Collaborator

@caitlinadams caitlinadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me -- I've added some minor comments for your consideration, but the code looks functional so I'm happy to approve.

asf_user=asf_user,
asf_password=asf_password,
)
try:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but I'm wondering if eof.download.main could use a tidy up? It's a bit unintuitive that it takes both CDSE and ASF credentials at this level, although that is explained with the comment. For readability, I would find it more intuitive if it attempted to download with CDSE credentials at the time where you check if source == "CDSE" -- however, I haven't looked at the function in detail, so there may be good reasons for having the downloader handle that logic.

asf_scene_metadata.download(path=download_folder, session=session)
if use_session:
asf_scene_metadata.download(path=download_folder, session=session)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a comment to explain that asf_scene_metadata will default to looking for the .netrc file if no session is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants