Общепринятый ключ в такого рода утилитах для рекурсивной обработки подкаталогов -r/--recursive. Ваш вариант интуитивно не очень понятный.
Сразу хочется придраться к наименованию основной функции и переменных. view_folder - подразумевает по смыслу некое неизменяющее воздействие, просмотр или что-то эдакое, но не то, что по факту делает её код. process_folder или clean_unpaired_raws подощло бы больше. А если добавить докстринг и тайпхинтинг для параметров, было бы вообще замечательно.
И да, параметры следует тоже называть консистентно. У вас одно и то же называется в программе сильно по-разному. Зачем вносить неразбериху? Зачем экономить несколько символов в ущерб понятности и читабельности?
Параметры следовало бы передавать по имени. Из-за неочевидного и неконсистентного именования приходится каждый раз смотреть что туда передаётся, причем внимательно следить, отсчитывая параметры позиционно и читать что про соответствующий параметр пишется в описании аргпарсера. Код должен быть максимально прозрачным даже для дго, кто первый раз его видит.
Если уж вы запросили ревью, то буду цепляться по порядку без учета степени и значительности недочетов. Какие-то вещи вполне норм, но я бы сделал иначе, их тоже буду упоминать.
Например, я бы в main передавал вектор аргументов с умолчанием в виде None. При передаче None доставал бы вектор из sys.argv[1:], как это делает парсер аргументов внутри, а при передаче конкретного вектора, использовал бы его. Это помогло бы протестировать парсинг параметров, если до таких автотестов дошли бы руки.
То, что у вас в основной функции, принято называть спагетти-кодом. Следует разделять код на удобные для понимания и тестирования части.
Я бы вынес в отдельную функцию формирование итератора по файлам.
Отдельной безопасной функцией генерил бы дифф.
Отдельно бы этот дифф применял. Отдельно (и опционально) спрашивал бы подтверждения пользователя.
Часто от утилиты требуется "тихая" работа или работа без лишних вопросов к пользователю. Для этого даже применяются специальные ключи, вроде -y/--yes.
Почему бы не импортнуть
from pathlib import path вместо постоянного обращения
pathlib.Path? Где-то экономите букву, а где-то вот так вот...
Вот это условие:
pathlib.Path(str(file.parent/file.stem) + img).is_file()
Следовало бы записать так:
file.with_suffix(img).exists()
Это короче и правильнее. Есть нюанс. У вас с таким именем может быть не только файл, но и каталог. Об этом часто забывают. Я бы в таких неоднозначных ситуациях следовал более безопасному поведению, например сохранению рав-файла, если соответствующая картинка - не картинка, а вообще каталог.
if 'raw_file' in str(file): #Если файл ранне был перемещен в папку raw_file его не игнорируем
continue
Игнорируете файлы в каталоге, заданном "магической константой". А если у пользователя такие каталоги используются для других целей?
В идеале нужно хотя бы задекларировать такое поведение на уровне документации.
Я бы ввёл перечень подкаталогов, которые игнорируются при сканировании. Позволил бы указывать название каталога, куда скидывать равы, в параметрах утилиты, добавлял бы это имя в список игнорируемых пол умолчанию каталогов.
Позволил бы список игнорируемых каталогов изменять.
Вы проверяете принадлежность фала подкаталогу проверкой вхождениея подстроки в его полное имя, но это неправильно, ведь такой кусок строки может быть в составе другого каталога или файла.
Например в полном пути каталога, который нужно почистить будетет что-то вроде такого:
"/home/dave/please_clean_raw_files_here/vacations2020/my_camera/IMG1010.RAW"
Все равы в "/home/dave/please_clean_
raw_files_here/vacations2020/my_camera/" будут проигнорированы.
Неожиданно, правда?
Ваша утилита будет работать гораздо быстрее, если сперва просканирует и запомнит все имена файлов, с которыми ей предстоит иметь дело, а потом будет оперировать ими в памяти.
Часто JPG файлы чистят уже после того, как равы отложили в подкаталог или соседнюю папку.
Ваша тулза умеет искать JPG только рядом с равом и грохнет все равы, для которых не найдёт JPGи, которые, впоне возможно, лежат в соседней папке. Вкупе с ключем рекурсивной обработки это может наделать бед внеся неразбериху и поудаляв нужные фотки.
Часто у рав-файла и JPG одна и та же дата-время в свойствах файла или в exif. Это позволит навярняка удалять только гарантированно непарные файлы, и вы не зацепите случаи, когда у камеры сбросился или провернулся счетчик кадров. Также решится проблема с ситуацией, когда в одном каталоге есть фотосеты с разных камер, которыые именуют фотки аналогичным образом. Очень трудно совпасть по времени кадра до долей секунд.
Вот в таких сучаях следует не следует инвертировать условие и углублять код, превращая его в спагетти:
if not len(difference) == 0:
folder = str()
print('\nRaw without jpg:')
for file in difference:
if not folder == file.parent:
folder = file.parent
print(f'\nFolder: {folder}')
print(f'- {file.name}')
else:
sys.exit(f'\nThere are no suitable files in the {path} folder\n')
Смотрите, если по одной из веток происходит выход, то его нужно сделать сразу, чтобы не забивать голову лишними отступами и уровнями ветвления:
if not difference: # да в питоне так проверяют пустоту списка
sys.exit(f'\nThere are no suitable files in the {path} folder\n')
А ещё обратите внимание, что '\nRaw without jpg:' выведется даже если ключи заставляют утилиту удалить все равы, даже те, к которым есть парные картинки. То есть ваша тулза откровенно врёт пользователю в некоторых ситуациях.
Вот здесь вы фактически группируете файлы по каталогам:
for file in difference:
if not folder == file.parent:
folder = file.parent
print(f'\nFolder: {folder}')
print(f'- {file.name}')
Отчего бы не вынести это в отдельную функцию, чтобы не захламлять код?
Да, она использовалась бы лишь единожды, но будучи понятно названной и понятно задокументированной эта функция сделает ваш код гораздо прозрачнее. Её вызов будет нести понятные последствия, а из спагетти-кода главной функции уйдёт приличный кусок, он будет заменен одним вызовом.
Обратите внимание на tertools.groupby. Она умеет группировать последовательность по результату лямбды и вы сможете сделать генератор генерторов, которые будут выдавать вам списки файлов по каталогам.
Пчему бы не заменить все вот такие куски на отдельную функцию с булевским результатом?
while True:
confirm = input('\nMove this files to a folder "raw_files"? press: y/n: ')
[...]
Пусть мучает пользователя бесконечно, пусть сделает системный выход, если приспичит, а лучше вальнётся с исключением, чтобы можно было при ее применении освободить ресурсы по finally.
Зато её вызов был бы простым и лаконичным:
if confirm('\nMove this files to a folder "raw_files"? press: y/n: '):
move_file(difference, difference_folder)
sys.exit('Complete\n')
Почему у вас функция move_file называется так, будто перемещает один файл, аргумент у нее наывается в единственном числе, а принимает она туа целый список путей?!!
Это либо злонамеренность, либо халатность. Нельзя так относиться к коду. Вы его пишете и на ходу меняете не думая о названиях, не думая о документации, не думая о консистентности. Полагаю раньше эта функция отвечала за перемещение только одого файла, а потмо вы вспороли ей брюхо и заставили обрабатывать список, но не позаботились ни о правильном названии, ни об изоляции... Франкенштейн у вас получился.
Дважды вычисляете одно и то же значение:
if not pathlib.Path(file.parent / 'raw_files' / f'{p.stem}({i}){file.suffix}').is_file():
pathlib.Path(file).replace(file.parent / 'raw_files' / f'{file.stem}({i}){file.suffix}')
У вас тут и магическая константа в коде на каждом шагу, и неправильно импортированный pathlib... Cнова забыли, что помимо файлов бывают каталоги и каталог с определённым именем просто сломают логику вашей тулзы.
Отсутствует корректная обработка ошибок. Открытый в каком-нибудь редакторе файл или папка с расширением .raw поломает к чертям всю работу и оставит её в некоторых случаях недоделанной, а это ещё хуже, чем вовсе не сделать ничего.
Зачем вы каждый раз делаете приведение к Path?
pathlib.Path(f / 'raw_files')
Результат конкатенации с Path будет тоже Path. Нет смысла приводить тип? И так в куче мест по всему коду!
Почему бы не сделать одну функцию по побработке одного единственного файла?
def move_raw_file(file: Path, dest_folder: typing.Union[Path, str None]=None, exists='safe'):
Переносит файл `file` в каталог dest_folder или удаляет, если он задан как None.
dest_folder может быть задан абсолютно или относительно родительского каталога file.
`exists`: safe, replace, ignore, error
В общем всё плохо. Учиться и учиться. Но бывает и хуже. так что дерзайте.
Для подобной утилиты ка кдля врача важно не навредить. Плохо будет, если вашу утилиту кто-то завернёт в GUI или засунет в пакетный файл научив неискушенного пользователя приводить в опрядок свою коллекцию фоточек одном кликом. В один прекрасный день с таким "качеством" написанная утилита перемешает фотки пользователя или, что хуже, удалит их часть.
Нужно использовать исключения, причем так, чтобы код становился прозрачнее и было понятно что произойдёт в каких случаях.
Нужно понимать как устроена файловая система и учитывать ее нюансы. Понимать, что файлы и папки - это немного разное, но они в едином пространстве имён. Понимать про абсолютные и относительные пути. Делать тулзы по умолчанию безопасными.
Понимать, что файлы могут быть поименованы произвольно, а вашей тулзе могут дать пожевать корневой каталог. Надо, чтобы она не загадила винт подкаталогами и не перетащила равы куда не следует.
