Skip to content

Conversation

@chrissnyder2337
Copy link
Contributor

@chrissnyder2337 chrissnyder2337 commented Jan 18, 2023

Brief Summary of Changes

Add the ability to upload the contents of a local directory without requiring that the parent directory (the local_folder) be created in Cloudinary.

Without this change, it is impossible to use cld upload_dir to upload a local directory's contents to Cloudinary without also creating a directory/folder within Cloudinary that also has the same name.

This was implemented by introducing new --exclude-dir-name (-e) command option that allowa the user to decide if the local parent directory should be created in Cloudinary, or just its contents.

Example without flag
Upload the local folder, my_images, and all its contents and sub-folders to the Cloudinary folder my_images_on_cloudinary, overwriting existing files. This example was pulled from the Cloudinary CLI documentation.

cld upload_dir -f my_images_on_cloudinary -o overwrite true my_images

This would result in the following folder structure in Cloudinary:

    Home
    +--my_images_on_cloudinary
    |  +-- my_images
    |  |   +-- <all assets and sub-folders of my_images>

Example using new -e (--exclude-dir-name) flag
Upload the contents of local folder, my_images, and all sub-folders to the Cloudinary folder my_images_on_cloudinary, overwriting existing files.

cld upload_dir -f my_images_on_cloudinary -e -o overwrite true my_images

This would result in the following folder structure in Cloudinary:

    Home
    +-- my_images_on_cloudinary
    |  +-- <all assets and sub-folders of my_images>

What does this PR address?

Are tests included?

  • Yes
  • No

Reviewer, please note:

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I ran the full test suite before pushing the changes and all the tests pass.

@const-cloudinary
Copy link
Contributor

@chrissnyder2337 Thank you for the PR! It is really a good use case.
We had an internal discussion about the parameter name and we got to the following:
It would be great if you could change the parameter name to:
--use-parent-dir and set it to True by default (current behavior)
Add a few tests under:
https://github.com/cloudinary/cloudinary-cli/blob/master/test/test_modules/test_cli_upload_dir.py

@chrissnyder2337
Copy link
Contributor Author

@const-cloudinary I have changed the parameter's name to --use-parent-dir and set it to default to true.

Due to the way that boolean flags work, and the desire to keep all existing functionality when the new command line option was not provided, I also needed to add a way to turn it off by adding a --dont-use-parent-dir(-P) option.

See https://click.palletsprojects.com/en/8.1.x/options/#boolean-flags

@chrissnyder2337
Copy link
Contributor Author

@const-cloudinary I have updated this PR's description to reflect the change of parameters.

@chrissnyder2337
Copy link
Contributor Author

@const-cloudinary I have just pushed an additional change to this PR to add a couple of tests and tweaked the implementation to account for a bug I found.

@chrissnyder2337
Copy link
Contributor Author

FYI: I have updated this PR's description to note the added tests.

@const-cloudinary
Copy link
Contributor

@chrissnyder2337 , thanks for the fixes and tests.
Let me just check with our PM team whether they are OK with the name of the parameter.

@const-cloudinary
Copy link
Contributor

@chrissnyder2337 , after another round of discussions, having a default parameter with a true value, which practically has no sense to keep, we came to this name:
-e, --exclude-dir-name, which removes the directory name.
Hope it's not too much work for you to change it.
Sorry for confusion.

@chrissnyder2337
Copy link
Contributor Author

@const-cloudinary I get it, naming things is hard.

I have changed the option to -e/--exclude-dir-name and updated the tests accordingly

Copy link
Contributor

@const-cloudinary const-cloudinary left a comment

Choose a reason for hiding this comment

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

Looks great!

Will wait for approval from our docs team.

Copy link
Contributor

@jackieros jackieros left a comment

Choose a reason for hiding this comment

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

@const-cloudinary Reviewed help texts.

@option("-t", "--transformation", help="The transformation to apply on all uploads.")
@option("-f", "--folder", default="",
help="The Cloudinary folder where you want to upload the assets. "
"You can specify a whole path, for example folder1/folder2/folder3. "
Copy link
Contributor

Choose a reason for hiding this comment

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

@const-cloudinary - Suggest changing this help text to:

The path where you want to upload the assets. The path you specify will be pre-pended to the public IDs of the uploaded assets. You can specify a whole path, for example path1/path2/path3.

@chrissnyder2337
Copy link
Contributor Author

@const-cloudinary @jackieros Let me know which, if not all, of the documentation suggestions to include in this PR.

const-cloudinary and others added 2 commits February 1, 2023 11:51
Co-authored-by: Jackie Rosenzveig <jackie@cloudinary.com>
Co-authored-by: Jackie Rosenzveig <jackie@cloudinary.com>
@const-cloudinary const-cloudinary merged commit df74cc4 into cloudinary:master Feb 1, 2023
@const-cloudinary
Copy link
Contributor

@chrissnyder2337 Thank you very much for your contribution!

@const-cloudinary
Copy link
Contributor

Fixes #64

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.

3 participants