fix: create recursive_from_prefix path if it does not exist #940

Bento007 · 2024-07-25T17:55:49Z

Reason

Improve error message when downloading to a new folder location #887

Changes

create the recursive_from_prefix directory if it does not exist.
add docstring to get_destination_path
add unit tests for get_destination_path

- expend on the existing error message to explain the cause. - add docstring to get_destination_path

Bento007 · 2024-07-25T19:14:17Z

client/python/cryoet_data_portal/src/cryoet_data_portal/_file_tools.py

    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.",


@kandarpksk what are your thoughts on the modified error message.

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.

manasaV3

LGTM

… tsmith/887-error-dest-path

manasaV3 · 2024-07-26T21:28:38Z

client/python/cryoet_data_portal/src/cryoet_data_portal/_file_tools.py

@@ -54,15 +54,27 @@ def download_https(

 def get_destination_path(
    url: str,
-    dest_path: Optional[str],
+    dest_path: Optional[str] = None,


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.

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.

manasaV3 · 2024-07-26T21:30:44Z

client/python/cryoet_data_portal/src/cryoet_data_portal/_file_tools.py

+    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


nit: Could we make this more clearer? Something like, "all files from this prefix are recursively downloaded" maybe?

manasaV3 · 2024-07-26T22:24:09Z

client/python/cryoet_data_portal/src/cryoet_data_portal/_file_tools.py

-    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)


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.

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>

fix: improve error message for missing dest_path

6058a2d

- expend on the existing error message to explain the cause. - add docstring to get_destination_path

Bento007 requested review from manasaV3 and kandarpksk July 25, 2024 17:55

Bento007 commented Jul 25, 2024

View reviewed changes

manasaV3 approved these changes Jul 25, 2024

View reviewed changes

Bento007 and others added 3 commits July 26, 2024 12:39

update test

372c99a

add unit test and create the recursive directory if it does not exist

d30ad39

Merge branch 'main' into tsmith/887-error-dest-path

71c87df

Bento007 requested a review from manasaV3 July 26, 2024 20:38

Bento007 added 5 commits July 26, 2024 13:40

update docstring

9253cf0

Merge remote-tracking branch 'origin/tsmith/887-error-dest-path' into…

b504449

… tsmith/887-error-dest-path

update tests to account for missing cwd

2470972

update tests to account for missing cwd

c223bf4

fix patch

e9046e4

Bento007 changed the title ~~fix: improve error message for missing dest_path~~ fix: create recursive_from_prefix path if it does not exist Jul 26, 2024

manasaV3 requested changes Jul 26, 2024

View reviewed changes

Bento007 added 4 commits July 26, 2024 16:02

update docstring, remove extra path creating, update tests.

d5f9f4f

force a failing directory correction.

5241c12

remove extra lines

dd64736

fix error message

7245fb5

manasaV3 approved these changes Jul 29, 2024

View reviewed changes

Bento007 merged commit 0069f08 into main Jul 29, 2024
15 checks passed

Bento007 deleted the tsmith/887-error-dest-path branch July 29, 2024 17:14

github-actions bot mentioned this pull request Jul 31, 2024

chore(tsmith/automate-release): release cryoet-data-portal-python-client 3.0.4 #975

Merged

github-actions bot mentioned this pull request Jul 31, 2024

chore(tsmith/automate-release): release cryoet-data-portal-python-client 3.0.4 #976

Merged

github-actions bot mentioned this pull request Jul 31, 2024

chore(tsmith/automate-release): release cryoet-data-portal-python-client 3.0.4 #981

Merged

github-actions bot mentioned this pull request Aug 2, 2024

chore(tsmith/automate-release): release cryoet-data-portal-python-client 3.1.0 #985

Closed

github-actions bot mentioned this pull request Aug 22, 2024

chore(main): release cryoet-data-portal-python-client 3.1.0 #1071

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: create recursive_from_prefix path if it does not exist #940

fix: create recursive_from_prefix path if it does not exist #940

Bento007 commented Jul 25, 2024 •

edited

Loading

Bento007 Jul 25, 2024

kandarpksk Jul 25, 2024 •

edited

Loading

manasaV3 left a comment

manasaV3 Jul 26, 2024

Bento007 Jul 26, 2024

manasaV3 Jul 26, 2024

manasaV3 Jul 26, 2024

Bento007 Jul 26, 2024 •

edited

Loading

fix: create recursive_from_prefix path if it does not exist #940

fix: create recursive_from_prefix path if it does not exist #940

Conversation

Bento007 commented Jul 25, 2024 • edited Loading

Reason

Changes

Bento007 Jul 25, 2024

Choose a reason for hiding this comment

kandarpksk Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

manasaV3 left a comment

Choose a reason for hiding this comment

manasaV3 Jul 26, 2024

Choose a reason for hiding this comment

Bento007 Jul 26, 2024

Choose a reason for hiding this comment

manasaV3 Jul 26, 2024

Choose a reason for hiding this comment

manasaV3 Jul 26, 2024

Choose a reason for hiding this comment

Bento007 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Bento007 commented Jul 25, 2024 •

edited

Loading

kandarpksk Jul 25, 2024 •

edited

Loading

Bento007 Jul 26, 2024 •

edited

Loading