Confusing options and possibly wrong branch logic

[bitsondatadev]

There's some code smell to this and I want to make sure this is actually the logic we want.
What if args.dont_fix = True and args.dont_copy=False and args.divide_to_dates=True?

We would hit the elif branch and call copy_to_target when args.divide_to_dates=True which doesn't seem to make sense.

    if not args.dont_fix:
        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=fix_metadata,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )
    if not args.dont_fix and not args.dont_copy and args.divide_to_dates:
        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=copy_to_target_and_divide,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )
    elif not args.dont_copy:
        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=copy_to_target,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )

Perhaps nesting the if logic under the first if was what was intended?

    if not args.dont_fix:
        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=fix_metadata,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )
        if not args.dont_copy and args.divide_to_dates:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target_and_divide,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )
        elif not args.dont_copy:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )

If this actually is correct then the args.dont_copy becomes redundant as args.dont_copy will always be the same as args.dont_fix and you could have this.

    if not args.dont_fix:
        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=fix_metadata,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )
        if args.divide_to_dates:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target_and_divide,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )
        else:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )

To be honest I don't think we should expose these as they are a bit confusing to anyone who can't understand the code, they make the code harder to understand, and as mentioned in the arguments themselves they seem a bit pointless.
--dont-fix

Don't try to fix Dates. I don't know why would you not want to do that, but ok

and
--dont-copy

Don't copy files to target folder. I don't know why would you not want to do that, but ok

If we removed these as options we could simply have this much clearer bit of code to read and people won't be scared of all these confusing levers they maybe aren't sure if they should pull.

        for_all_files_recursive(
            dir=PHOTOS_DIR,
            file_function=fix_metadata,
            filter_fun=lambda f: (is_photo(f) or is_video(f))
        )
        if args.divide_to_dates:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target_and_divide,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )
        else:
            for_all_files_recursive(
                dir=PHOTOS_DIR,
                file_function=copy_to_target,
                filter_fun=lambda f: (is_photo(f) or is_video(f))
            )

--skip-extras and --skip-extras-harder , while the # Oh yeah, skip my extras harder daddy comment is hilarious, also seems rather confusing to expose as an option and it's always better to err on the side on no data loss. Someone not able to read the code well may be tempted to use this option and be sad when things are gone.

[TheLastGimbus]

TL;DR This code is shit - I'm honestly sorry 😐

Perhaps nesting the if logic under the first if was what was intended? (+ the code block below)

You may want to just recursively copy without fixing...

--skip-extras and --skip-extras-harder

@DalenW wanted it in #12, and I think it's resonable to add such option

Someone not able to read the code well may be tempted to use this option and be sad when things are gone.

All boils down that I did not thought that it will need maitnance. Nevertheless, I think those options are very resonable - just not well documented, and there is no CONTRIBUTING.md and it's all confusing...

Honestly, I wish we will get the albums right, and I'll try to fix #30, and we will be done with expanding this script...

[TheLastGimbus]

Fixed with this commit