-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: create recursive_from_prefix path if it does not exist #940
Conversation
- expend on the existing error message to explain the cause. - add docstring to get_destination_path
if not dest_path: | ||
dest_path = os.getcwd() | ||
dest_path = os.path.abspath(dest_path) | ||
|
||
if not os.path.isdir(dest_path) and recursive_from_prefix: | ||
raise ValueError("Recursive downloads require a base directory") | ||
raise ValueError( | ||
f"The destination path '{dest_path}' does not exist. Recursive downloads require an existing base directory.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kandarpksk what are your thoughts on the modified error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read recent comments in the issue, and think we should just create the missing folder in all cases.
If we don't go with that, I find the updated message still confusing. I think the language we use should be from the perspective of the API user, who is either downloading a file (with say, download_mrcfile) or a folder (with say, download_everything). In which case is the download recursive? (I'd ordinarily assume that the folder download is recursive, but from Manasa's comment, it seems to me that in that case, this error is not thrown!)
Again, I don't think we should go with this option. If we do, let's also tag Dannielle to chime in about the message wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -54,15 +54,27 @@ def download_https( | |||
|
|||
def get_destination_path( | |||
url: str, | |||
dest_path: Optional[str], | |||
dest_path: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case, where we are calling this method without a value for dest_path
? If yes, we should fix that method call.
We don't want to support a usecase where we accidentally miss passing on the destination_path provided by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case, where we are calling this method without a value for dest_path?
It's only used in one place, and dest_path is provided.
I see, you're using Optional to mean String or None while at the same time enforcing that dest_path must be a parameter.
Args: | ||
url (str): The URL to download | ||
dest_path (str): The destination path the files will download to | ||
recursive_from_prefix (str): The prefix to use for recursive downloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could we make this more clearer? Something like, "all files from this prefix are recursively downloaded" maybe?
if not os.path.isdir(dest_path) and recursive_from_prefix: | ||
raise ValueError("Recursive downloads require a base directory") | ||
if not os.path.isdir(dest_path): | ||
os.makedirs(dest_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the dest_path is None, it should point to the working directory, and will pass this check. So, the only time this exception would show up would be when the user explicitly provides an invalid value and we should error on it. I think the error message could be more verbose in explaining the issue. cc: @kandarpksk
Also, this line should already be creating the recursive directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So error when an invalid path is provided? Invalid meaning illegal path characters or maybe a path we can't write to?
…ent 3.0.4 (#975) 🤖 I have created a release *beep* *boop* --- ## [3.0.4](cryoet-data-portal-python-client-v3.0.3...cryoet-data-portal-python-client-v3.0.4) (2024-07-31) ### 🐞 Bug Fixes * create recursive_from_prefix path if it does not exist ([#940](#940)) ([0069f08](0069f08)) * Use match with substring for exception check in client tests ([#895](#895)) ([07352ec](07352ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ent 3.0.4 (#976) 🤖 I have created a release *beep* *boop* --- ## [3.0.4](cryoet-data-portal-python-client-v3.0.3...cryoet-data-portal-python-client-v3.0.4) (2024-07-31) ### 🐞 Bug Fixes * add link to documentation to pypi ([38c76b4](38c76b4)) * create recursive_from_prefix path if it does not exist ([#940](#940)) ([0069f08](0069f08)) * Use match with substring for exception check in client tests ([#895](#895)) ([07352ec](07352ec)) ### 🧹 Miscellaneous Chores * automate pypi release ([508516b](508516b)) * prepare client for release automation ([d7e5446](d7e5446)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com>
…ent 3.0.4 (#981) 🤖 I have created a release *beep* *boop* --- ## [3.0.4](cryoet-data-portal-python-client-v3.0.3...cryoet-data-portal-python-client-v3.0.4) (2024-07-31) ### 🐞 Bug Fixes * add link to documentation to pypi ([38c76b4](38c76b4)) * create recursive_from_prefix path if it does not exist ([#940](#940)) ([0069f08](0069f08)) * Use match with substring for exception check in client tests ([#895](#895)) ([07352ec](07352ec)) ### 🧹 Miscellaneous Chores * automate pypi release ([508516b](508516b)) * prepare client for release automation ([d7e5446](d7e5446)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [3.1.0](cryoet-data-portal-python-client-v3.0.3...cryoet-data-portal-python-client-v3.1.0) (2024-08-22) ### ✨ Features * add user agent to client requests ([#966](#966)) ([8209cd4](8209cd4)) * Generate Python client code using GraphQL introspection ([#1008](#1008)) ([35b7265](35b7265)) ### 🐞 Bug Fixes * create recursive_from_prefix path if it does not exist ([#940](#940)) ([0069f08](0069f08)) * Use match with substring for exception check in client tests ([#895](#895)) ([07352ec](07352ec)) * wait for graphql to be healthy in client tests ([#1044](#1044)) ([65f0a4b](65f0a4b)), closes [#942](#942) ### 🧹 Miscellaneous Chores * Add additional test case to TestGetDestinationPath ([#955](#955)) ([a9412a8](a9412a8)) * add instructions and commands to manually release the python package. ([#1073](#1073)) ([4833eb9](4833eb9)) * automate release of python client ([#972](#972)) ([073bff7](073bff7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Reason
Changes
get_destination_path
get_destination_path