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

Add command line arguments to filter by date. #779

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

itwasabhi
Copy link

With the new arguments --date-before and --date-after, user can choose to only download items that are created either before, after, or before and after the specified dates. Dates are assumed to be specified in the local timezone.

This should address issue #466, something I have also wanted for a while!

Comments / suggestions welcome.

With the new argments --date-before and --date-after, user can choose
to only download items that are created either before, after, or before
and after the specified dates. Dates are assumed to be specified in
the local timezone.
tests/test_download_photos.py Show resolved Hide resolved
tests/test_download_photos.py Outdated Show resolved Hide resolved
src/icloudpd/base.py Outdated Show resolved Hide resolved
@itwasabhi
Copy link
Author

@AndreyNikiforov should be ready for re-review

files_in_result = glob.glob(os.path.join(
data_dir, "**/*.*"), recursive=True)

assert sum(1 for _ in files_in_result) == 0, "No files should have been downloaded."
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls adjust the test to have a set of files to download with different created dates and then "cut" that set with your new parameter. Confirm that only desired portion of the set is downloaded. That should confirm that criteria filtering is correct and test itself is not broken (as it downloads some files).

There should be tests that validate filters in this manner.

@@ -418,6 +452,18 @@ def download_photo_(counter: Counter, photo: PhotoAsset) -> bool:
photo.created)
created_date = photo.created

if created_before and created_date > created_before:
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are the conditions we need to validate with a test. Parameter absence is tested, but comparison is not completely validated IMO - need to check we indeed leave some assets and filter out others (assets created on the same date as parameter is the case that should be present in tests as well). In e2e tests that will be translated into checking that some files are downloaded and some are not.

@mrsilver76
Copy link

@itwasabhi Is there any chance you can do the changes requested? It's a useful feature so thank you for developing it.

@itwasabhi
Copy link
Author

@mrsilver76 Sure, please give me a week or so to sync to head and make the requested changes!

@fliespl
Copy link

fliespl commented Oct 27, 2024

@itwasabhi do you think you will get back to it somewhere in the future :) ? This would a game-changer for date based deletion.

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.

4 participants