Skip to content

Commit f42639c

Browse files
refactor: optimize filename building with factory pattern and required parameters
• Create filename_policies.py with create_filename_builder() factory using partial_2_1 • Replace 12 instances of duplicated filename building code with single function • Remove None optionality from file_match_policy and filename_cleaner parameters • Eliminate redundant parameters from download_media function • Fix parameter order bugs in delete function calls • Update test assertions to match new function signatures Performance improvements: - Pre-configured filename builders eliminate repeated parameter passing - No runtime None checks in hot paths with required parameters - Better type safety with guaranteed non-None values - Reduced function complexity by removing defensive programming branches 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 4747fe4 commit f42639c

File tree

5 files changed

+101
-177
lines changed

5 files changed

+101
-177
lines changed

src/icloudpd/base.py

Lines changed: 39 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
from icloudpd.config import GlobalConfig, UserConfig
4545
from icloudpd.counter import Counter
4646
from icloudpd.email_notifications import send_2sa_notification
47+
from icloudpd.filename_policies import build_filename_with_policies, create_filename_builder
4748
from icloudpd.log_level import LogLevel
4849
from icloudpd.mfa_provider import MFAProvider
4950
from icloudpd.password_provider import PasswordProvider
@@ -70,9 +71,6 @@
7071
PhotoAlbum,
7172
PhotoAsset,
7273
PhotoLibrary,
73-
apply_file_match_policy,
74-
apply_filename_cleaner,
75-
filename_with_fallback,
7674
)
7775
from pyicloud_ipd.utils import (
7876
disambiguate_filenames,
@@ -382,6 +380,11 @@ def password_provider(_username: str) -> str | None:
382380
# Set up filename cleaner based on user preference
383381
filename_cleaner = build_filename_cleaner(user_config.keep_unicode_in_filenames)
384382

383+
# Create filename builder with pre-configured policy and cleaner
384+
filename_builder = create_filename_builder(
385+
user_config.file_match_policy, filename_cleaner
386+
)
387+
385388
# Set up function builders
386389
passer = partial(
387390
where_builder,
@@ -390,8 +393,7 @@ def password_provider(_username: str) -> str | None:
390393
user_config.skip_created_before,
391394
user_config.skip_created_after,
392395
user_config.skip_photos,
393-
user_config.file_match_policy,
394-
filename_cleaner,
396+
filename_builder,
395397
)
396398

397399
downloader = (
@@ -410,7 +412,7 @@ def password_provider(_username: str) -> str | None:
410412
user_config.file_match_policy,
411413
user_config.xmp_sidecar,
412414
lp_filename_generator,
413-
filename_cleaner,
415+
filename_builder,
414416
user_config.align_raw,
415417
)
416418
if user_config.directory is not None
@@ -516,35 +518,26 @@ def where_builder(
516518
skip_created_before: datetime.datetime | datetime.timedelta | None,
517519
skip_created_after: datetime.datetime | datetime.timedelta | None,
518520
skip_photos: bool,
519-
file_match_policy: FileMatchPolicy,
520-
filename_cleaner: Callable[[str], str],
521+
filename_builder: Callable[[PhotoAsset], str],
521522
photo: PhotoAsset,
522523
) -> bool:
523524
if skip_videos and photo.item_type == AssetItemType.MOVIE:
524-
logger.debug(asset_type_skip_message(photo, file_match_policy, filename_cleaner))
525+
logger.debug(asset_type_skip_message(photo, filename_builder))
525526
return False
526527
if skip_photos and photo.item_type == AssetItemType.IMAGE:
527-
logger.debug(asset_type_skip_message(photo, file_match_policy, filename_cleaner))
528+
logger.debug(asset_type_skip_message(photo, filename_builder))
528529
return False
529530

530531
if skip_created_before is not None:
531532
temp_created_before = offset_to_datetime(skip_created_before)
532533
if photo.created < temp_created_before:
533-
logger.debug(
534-
skip_created_before_message(
535-
temp_created_before, photo, file_match_policy, filename_cleaner
536-
)
537-
)
534+
logger.debug(skip_created_before_message(temp_created_before, photo, filename_builder))
538535
return False
539536

540537
if skip_created_after is not None:
541538
temp_created_after = offset_to_datetime(skip_created_after)
542539
if photo.created > temp_created_after:
543-
logger.debug(
544-
skip_created_after_message(
545-
temp_created_after, photo, file_match_policy, filename_cleaner
546-
)
547-
)
540+
logger.debug(skip_created_after_message(temp_created_after, photo, filename_builder))
548541
return False
549542

550543
return True
@@ -553,38 +546,18 @@ def where_builder(
553546
def skip_created_before_message(
554547
target_created_date: datetime.datetime,
555548
photo: PhotoAsset,
556-
file_match_policy: FileMatchPolicy | None = None,
557-
filename_cleaner: Callable[[str], str] | None = None,
549+
filename_builder: Callable[[PhotoAsset], str],
558550
) -> str:
559-
if file_match_policy is not None and filename_cleaner is not None:
560-
# Compose calculate_filename with cleaning and file match policy transformations
561-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
562-
raw_filename = extract_with_fallback(photo.calculate_filename())
563-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
564-
cleaned_filename = filename_cleaner_transformer(raw_filename)
565-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
566-
filename = policy_transformer(cleaned_filename)
567-
else:
568-
filename = photo.filename
551+
filename = filename_builder(photo)
569552
return f"Skipping {filename}, as it was created {photo.created}, before {target_created_date}."
570553

571554

572555
def skip_created_after_message(
573556
target_created_date: datetime.datetime,
574557
photo: PhotoAsset,
575-
file_match_policy: FileMatchPolicy | None = None,
576-
filename_cleaner: Callable[[str], str] | None = None,
558+
filename_builder: Callable[[PhotoAsset], str],
577559
) -> str:
578-
if file_match_policy is not None and filename_cleaner is not None:
579-
# Compose calculate_filename with cleaning and file match policy transformations
580-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
581-
raw_filename = extract_with_fallback(photo.calculate_filename())
582-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
583-
cleaned_filename = filename_cleaner_transformer(raw_filename)
584-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
585-
filename = policy_transformer(cleaned_filename)
586-
else:
587-
filename = photo.filename
560+
filename = filename_builder(photo)
588561
return f"Skipping {filename}, as it was created {photo.created}, after {target_created_date}."
589562

590563

@@ -602,7 +575,7 @@ def download_builder(
602575
file_match_policy: FileMatchPolicy,
603576
xmp_sidecar: bool,
604577
lp_filename_generator: Callable[[str], str],
605-
filename_cleaner: Callable[[str], str],
578+
filename_builder: Callable[[PhotoAsset], str],
606579
raw_policy: RawTreatmentPolicy,
607580
icloud: PyiCloudService,
608581
counter: Counter,
@@ -664,13 +637,7 @@ def download_builder(
664637
for download_size in primary_sizes:
665638
if download_size not in versions and download_size != AssetVersionSize.ORIGINAL:
666639
if force_size:
667-
# Compose calculate_filename with cleaning and file match policy transformations
668-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
669-
raw_filename = extract_with_fallback(photo.calculate_filename())
670-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
671-
cleaned_filename = filename_cleaner_transformer(raw_filename)
672-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
673-
error_filename = policy_transformer(cleaned_filename)
640+
error_filename = filename_builder(photo)
674641
logger.error(
675642
"%s size does not exist for %s. Skipping...",
676643
download_size.value,
@@ -682,13 +649,7 @@ def download_builder(
682649
download_size = AssetVersionSize.ORIGINAL
683650

684651
version = versions[download_size]
685-
# Compose calculate_filename with cleaning and file match policy transformations
686-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
687-
raw_filename = extract_with_fallback(photo.calculate_filename())
688-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
689-
cleaned_filename = filename_cleaner_transformer(raw_filename)
690-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
691-
photo_filename = policy_transformer(cleaned_filename)
652+
photo_filename = filename_builder(photo)
692653
filename = calculate_version_filename(
693654
photo_filename,
694655
version,
@@ -738,8 +699,7 @@ def download_builder(
738699
download_path,
739700
version,
740701
download_size,
741-
file_match_policy,
742-
filename_cleaner,
702+
filename_builder,
743703
)
744704
success = download_result
745705

@@ -776,13 +736,7 @@ def download_builder(
776736
photo_versions_with_policy = photo.versions_with_raw_policy(raw_policy)
777737
if lp_size in photo_versions_with_policy:
778738
version = photo_versions_with_policy[lp_size]
779-
# Compose calculate_filename with cleaning and file match policy transformations
780-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
781-
raw_filename = extract_with_fallback(photo.calculate_filename())
782-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
783-
cleaned_filename = filename_cleaner_transformer(raw_filename)
784-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
785-
lp_photo_filename = policy_transformer(cleaned_filename)
739+
lp_photo_filename = filename_builder(photo)
786740
lp_filename = calculate_version_filename(
787741
lp_photo_filename,
788742
version,
@@ -843,8 +797,7 @@ def download_builder(
843797
lp_download_path,
844798
version,
845799
lp_size,
846-
file_match_policy,
847-
filename_cleaner,
800+
filename_builder,
848801
)
849802
success = download_result and success
850803
if download_result:
@@ -856,20 +809,10 @@ def delete_photo(
856809
logger: logging.Logger,
857810
library_object: PhotoLibrary,
858811
photo: PhotoAsset,
859-
file_match_policy: FileMatchPolicy | None = None,
860-
filename_cleaner: Callable[[str], str] | None = None,
812+
filename_builder: Callable[[PhotoAsset], str],
861813
) -> None:
862814
"""Delete a photo from the iCloud account."""
863-
if file_match_policy is not None and filename_cleaner is not None:
864-
# Compose calculate_filename with cleaning and file match policy transformations
865-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
866-
raw_filename = extract_with_fallback(photo.calculate_filename())
867-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
868-
cleaned_filename = filename_cleaner_transformer(raw_filename)
869-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
870-
clean_filename_local = policy_transformer(cleaned_filename)
871-
else:
872-
clean_filename_local = photo.filename
815+
clean_filename_local = filename_builder(photo)
873816
logger.debug("Deleting %s in iCloud...", clean_filename_local)
874817
url = (
875818
f"{library_object.service_endpoint}/records/modify?"
@@ -901,20 +844,10 @@ def delete_photo_dry_run(
901844
logger: logging.Logger,
902845
library_object: PhotoLibrary,
903846
photo: PhotoAsset,
904-
file_match_policy: FileMatchPolicy | None = None,
905-
filename_cleaner: Callable[[str], str] | None = None,
847+
filename_builder: Callable[[PhotoAsset], str],
906848
) -> None:
907849
"""Dry run for deleting a photo from the iCloud"""
908-
if file_match_policy is not None and filename_cleaner is not None:
909-
# Compose calculate_filename with cleaning and file match policy transformations
910-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
911-
raw_filename = extract_with_fallback(photo.calculate_filename())
912-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
913-
cleaned_filename = filename_cleaner_transformer(raw_filename)
914-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
915-
filename = policy_transformer(cleaned_filename)
916-
else:
917-
filename = photo.filename
850+
filename = filename_builder(photo)
918851
logger.info(
919852
"[DRY RUN] Would delete %s in iCloud library %s",
920853
filename,
@@ -931,21 +864,11 @@ def dump_responses(dumper: Callable[[Any], None], responses: List[Mapping[str, A
931864

932865
def asset_type_skip_message(
933866
photo: PhotoAsset,
934-
file_match_policy: FileMatchPolicy | None = None,
935-
filename_cleaner: Callable[[str], str] | None = None,
867+
filename_builder: Callable[[PhotoAsset], str],
936868
) -> str:
937869
# reverse logic assumes only two options
938870
photo_video_phrase = "photos" if photo.item_type == AssetItemType.MOVIE else "videos"
939-
if file_match_policy is not None and filename_cleaner is not None:
940-
# Compose calculate_filename with cleaning and file match policy transformations
941-
extract_with_fallback = filename_with_fallback(photo.id, photo.item_type_extension)
942-
raw_filename = extract_with_fallback(photo.calculate_filename())
943-
filename_cleaner_transformer = apply_filename_cleaner(filename_cleaner)
944-
cleaned_filename = filename_cleaner_transformer(raw_filename)
945-
policy_transformer = apply_file_match_policy(file_match_policy, photo.id)
946-
filename = policy_transformer(cleaned_filename)
947-
else:
948-
filename = photo.filename
871+
filename = filename_builder(photo)
949872
return f"Skipping {filename}, only downloading {photo_video_phrase}. (Item type was: {photo.item_type})"
950873

951874

@@ -1180,23 +1103,11 @@ def should_break(counter: Counter) -> bool:
11801103
filename_cleaner_for_debug = build_filename_cleaner(
11811104
user_config.keep_unicode_in_filenames
11821105
)
1183-
# Compose calculate_filename with file match policy transformation
1184-
extract_with_fallback = filename_with_fallback(
1185-
item.id, item.item_type_extension
1186-
)
1187-
raw_filename = extract_with_fallback(
1188-
item.calculate_filename()
1189-
)
1190-
filename_cleaner_transformer = apply_filename_cleaner(
1191-
filename_cleaner_for_debug
1192-
)
1193-
cleaned_filename = filename_cleaner_transformer(
1194-
raw_filename
1195-
)
1196-
debug_policy_transformer = apply_file_match_policy(
1197-
user_config.file_match_policy, item.id
1106+
debug_filename = build_filename_with_policies(
1107+
user_config.file_match_policy,
1108+
filename_cleaner_for_debug,
1109+
item,
11981110
)
1199-
debug_filename = debug_policy_transformer(cleaned_filename)
12001111
logger.debug(
12011112
"Skipping deletion of %s as it is within the keep_icloud_recent_days period (%d days old)",
12021113
debug_filename,
@@ -1206,25 +1117,26 @@ def should_break(counter: Counter) -> bool:
12061117
should_delete = True
12071118

12081119
if should_delete:
1209-
# Create filename cleaner for delete operations
1120+
# Create filename cleaner and builder for delete operations
12101121
filename_cleaner_for_delete = build_filename_cleaner(
12111122
user_config.keep_unicode_in_filenames
12121123
)
1124+
filename_builder_for_delete = create_filename_builder(
1125+
user_config.file_match_policy, filename_cleaner_for_delete
1126+
)
12131127
if user_config.dry_run:
12141128
delete_photo_dry_run(
12151129
logger,
12161130
library_object,
12171131
item,
1218-
user_config.file_match_policy,
1219-
filename_cleaner_for_delete,
1132+
filename_builder_for_delete,
12201133
)
12211134
else:
12221135
delete_photo(
12231136
logger,
12241137
library_object,
12251138
item,
1226-
user_config.file_match_policy,
1227-
filename_cleaner_for_delete,
1139+
filename_builder_for_delete,
12281140
)
12291141

12301142
# retrier(delete_local, error_handler)

0 commit comments

Comments
 (0)