Skip to content
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

Make Dropbox listdir optionally recursive #3260

Closed

Conversation

smrohrer
Copy link
Contributor

Description

Make the Dropbox listdir function accept an optional keyword argument recursive, True by default to preserve the current behavior.

Motivation and Context

Currently, listdir calls the Dropbox SDK files_list_folder function with recursive=True hard-coded. recursive should be optional, so we can replicate the functionality of LocalFileSystem.listdir which is not recursive.

Have you tested this? If so, how?

  • Added a unit test for non-recursive behavior
  • Modified the other listdir tests to verify that by default, the list includes the nested file DROPBOX_TEST_FILE_IN_DIR
  • Ran tox -e py37-dropbox locally

@smrohrer smrohrer requested review from dlstadther and a team as code owners October 28, 2023 16:14
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Code change and tests lgtm, with 1 confusing test assertion. I assume it's an underlying sdk implementation, but i would like to doublecheck.

def test_listdir_non_recursive(self):
list_of_dirs = self.luigiconn.listdir(DROPBOX_TEST_PATH, recursive=False)
self.assertTrue('/' not in list_of_dirs)
self.assertTrue(DROPBOX_TEST_PATH not in list_of_dirs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused why the input path appears in the results during recursive=True, but not when recursive=False. Is that expected behavior for the underlying api call?

@smrohrer
Copy link
Contributor Author

Thank you for the quick and diligent review.

I am closing this pull request because of a misunderstanding on my part: I confused the intended behavior of luigi.target.FileSystem.listdir with os.listdir - the former is meant to be recursive, and the latter is not.

However, I think the inclusion of the input path in the Dropbox listdir result does contradict the expected behavior of luigi.target.FileSystem.listdir. This is a behavior of the Dropbox files_list_folder function which is not described in the documentation.

If you think luigi.contrib.dropbox.DropboxClient.listdir should filter out the input path, I'd be happy to open another pull request.

@smrohrer smrohrer closed this Oct 29, 2023
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