Download file with a directory specified by local_path parameter

[snbianco]

Fixes #2501

Appends filename to a local_path argument to avoid IsADirectoryError. Also updated documentation to be a bit more clear about the function and the parameter.

[pep8speaks]

Hello @snbianco! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-06-11 16:15:11 UTC

[snbianco]

@bsipocz

Hi! I'm Sam, a new developer at STScI working with @astrojimig. This is a PR to fix a bug in one of our methods.

[astrojimig]

I'm wondering if we want to add some logic here that if the local_path directory doesn't exist, the directory is created? Or is that already covered elsewhere?

[snbianco]

I was kind of wondering about this too... right now, a directory that doesn't exist gets created as a file in the pwd since the check on line 539 returns false. I considered checking if the string contains a . character to tell if it's a file, but we would then run into issues with paths like this: ./tmp/

Would it make more sense to check that the string contains a ., but that it isn't the leading character?

[astrojimig]

I think there's some logic with pathlib that you could use for this!

Something like:

local_path = pathlib.Path(local_path)
if not local_path.exists():
        local_path.mkdir(parents=True, exist_ok=True)

[snbianco]

Sweet! Most recent version creates a Path object and differentiates between a file and a directory by checking path.suffix. If a file extension isn't found, then it's assumed that local_path is meant to point to a directory. If the directory does not exist, it's then created.

[astrojimig]

Thanks, this looks great!

[astrojimig]

It also might need a second elif to finish this off: in the case that local_path is supplied as a filename?

[snbianco]

In the most recent changes, I differentiate between a filename and a directory by checking if the pathlib.Path object has an extension. I don't think we should need a branch to check that local_path is a filename since the parameter shouldn't need any further processing and can be passed directly into the download_file function.

[bsipocz]

Thanks!

Content-wise it looks good, but can I ask you to rebase the PR to: 1) remove the merge commit 2) to consolidate the commits to a few logical one by e.g. squashing the style fixing iterations, changelog iterations, etc. No need to squash down to one, but eliminate the incremental iterations.

[bsipocz]

minor note: unfortunately "address" is not one of the verbs that links an issue to a PR, pick one of close/fix/resolve instead :) https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#about-linked-issues-and-pull-requests

[bsipocz]

Thanks, this all looks good now, and no new test failure is present.