jmathai / elodie

An EXIF-based photo assistant, organizer and workflow automation tool.
https://bit.ly/introducing-elodie
Apache License 2.0
1.27k stars 139 forks source link

Introduce skipping #403

Closed peterrus closed 2 years ago

peterrus commented 3 years ago

Why

I have a specific use case where I want to run Elodie's import on a schedule (through cron) on a folder where my mobile phone syncs taken pictures to. I want the pictures to stay in the synced folder but want to run Elodie nightly to process any new pictures into my main picture library (which is already sorted by Elodie).

Currently Elodie already skips pictures that are already present in the library. You can see this happen by running Elodie with the --verbose flag. These skipped pictures are however counted as errors in the Result object. I would argue that a failure to import a picture because it is a duplicate should not be counted as an error but rather as a notice, or in my implementation, a 'skip'.

What

I have coded a quick and dirty proof of concept of the behavior i'd like to have: When a picture is detected as duplicate it is being skipped, as was the default behavior, but the skip is counted as an actual skip in the Result and not as an error.

I am considering creating an additional PR that returns a non-zero status code when Result contains >1 error. That would mean however that i'd have to review all other instances where something is counted as error, and decide if it would be considered a 'skip' (which does not trigger the return of a nonzero status code) or that we'd have to add another counter in Result (I am thinking of 'warning' or 'notice', which might supersede 'skip' althogether).

I believe this changes will make Elodie more suitable to be run in automated environments. I could have my cronjob or script decide whether or not a message needs to be sent to the server admin (me) to intervene in case of an actual error.

Request for comments

I'd like to ask about your opinions on this solution and how we can perfect it.

Further

Unit tests are currently failing, but I intend to fix them if this PR takes a more form.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

jmathai commented 3 years ago

Thanks for the PR.

I think this feature should be implemented to augment allow_duplicates.

If allow_duplicates == False then an additional parameter can determine if a duplicate file is considered a success or error.

pseudocode(is_duplicate, allow_duplicates, success_on_duplicate)
  if(is_duplicate)
    if(allow_duplicates)
      if(success_on_duplicate)
        // success
      else
        // error
    else
      // success
  else
    // success

Thoughts?

jmathai commented 3 years ago

The above logic should prevent current tests from failing and we should be able to add new tests to verify it's working correctly.