The move function operation flags are error prone

[zigarrre]

Problem

As I'm in the process of implementing #2629 I noticed something else: The current move functions are somewhat ambiguous and error prone. According to the code only one of the operations (move, copy, link, hardlink) is possible at a time. So the operation should be passed as a single parameter instead of a set of flags.

Consider the following function call that makes no sense and will lead to errors but looks fine:

x.move(copy=True, link=True)

A good API should be hard to use incorrectly.

Proposed solution

Define an enum or constants (of either integers or strings) describing the possible operations and change all move functions to accept the operation as a single operator.

For example a enum based solution could look like the following:

class MoveOperations(Enum):
    MOVE = 0
    COPY = 1
    LINK = 2
    HARDLINK = 3

def move(self, operation, ...)  # change all the move, move_item and move_art functions

x.move(MoveOperations.COPY)

If you like the idea just let me know what variant you like most (enum or module wide constants with integer or string values) and I will refactor the code and send you a pull request.

[sampsyo]

Absolutely; this is a good refactoring idea. (The backstory here is essentially the same as the importer options: this made sense when the only alternative was "copy," but we've outlived that old design.)

It probably makes sense to do this first, before all of #2629, so the importer can use the same enum.

I do think an Enum class is probably the right thing. One tiny nit, though: I think the naming convention would suggest the singular MoveOperation instead of the plural MoveOperations.

[zigarrre]

Doing this first and using the same enum for the events was my plan. I didn't notice till now that the importer could also use this refactoring on some functions, good that you mentioned that.

I noticed that multiple methods only check the copy flag before attempting to prune the source directory (e.g. here). Is this a remnant from when move and copy where the only options? As far as I can tell pruning doesn't make sense when linking or hardlinking so I'll refactor that to operation == MoveOperation.MOVE.

[sampsyo]

Yes, that's a good point—I think that is a holdover from when those were the only two options. Everything else should be considered "copy-like" for the purpose of pruning.

[zigarrre]

OK.

I have finished most of the refactoring but I'm unsure what to do with ImportTask.manipulate_files. I'm not sure if I understand that correctly but it seems like it's a valid use to pass it all flags false which should result in no moving at all. If that's the case I can't refactor it this way as there is no MoveOperation for "doing nothing" and I don't want to introduce one as that would be an invalid value for the move functions.

Also I thought about changing the [move, copy, link, hardlink] config options in ImportSession.config to a single option called operation but I'm not sure if that's a good idea.

What's your opinion on those two things and what do you want me to do?

[sampsyo]

That's a good point. Yes, it is possible for manipulate_files to do nothing to your files—that's what you get now, for example, if you just set copy: no in your config file. (This is useful if you already have a manually-managed directory structure and you just want to keep it.) So perhaps the parameter for that method should accept a MoveOperation or None to indicate that the files should be kept "in place."

About the configuration option for the import section: I'm actually not sure which is more obvious for users. Would files: copy or something be easier to read than copy: yes? Possibly! In any case, it would be nice to keep the current options for compatibility—maybe we should start there and then worry about the new configuration interface in a subsequent effort?

[apitofme]

Should this issue be closed?

Looking in importer.py:

def manipulate_files(self, operation=None, write=False, session=None):
        """ Copy, move, link or hardlink (depending on `operation`) the files
        as well as write metadata.

        `operation` should be an instance of `util.MoveOperation`.

        If `write` is `True` metadata is written to the files.
        """

...with regards to the comment...

sampsyo commented on 10 Sep 2017
...
So perhaps the parameter for that method should accept a MoveOperation or None to indicate that the files should be kept "in place." ...

...suggests that the solution was successfully implemented?!

---

As for the config option, FWIW I agree that files: copy (or move/link etc.) would be clearer, but I understand wanting to maintain compatibility.

[sampsyo]

Yep! This is done. Refactoring the config (in a backwards compatible way) is of course still an option!