Adds meta flag 'skip_diff' to enable skipping of diff operations

[matthewhegarty]

Problem

When importing a large number of rows (using this patch), disabling the diff operation results in ~30% improvement in import time.

Solution

This PR involves adding a meta attribute to the Resource, which gives the option of disabling diffing operations. The default value is False meaning existing behaviour is unchanged unless the flag is explicitly set to True.

Acceptance Criteria

There is a new TestCase in test_resources.py which uses patches to verify that diff and copy functions are not called.

The flag is documented in resources.py.

[coveralls]

Coverage increased (+0.1%) to 96.243% when pulling dd342f5 on matthewhegarty:skip-diff into 7c58899 on django-import-export:master.

[manelclos]

@matthewhegarty I don't understand why you are adding not skip_diff here, can you please explain?

[matthewhegarty]

This is an optimisation because at this point (if skip_diff is True), the original var is not set.

If we allow skip_row() to run then (assuming skip_unchanged is True) it will iterate over all model fields, and throw AttributeError for each when it tries to read a value from a None reference.

I could potentially move the if not skip_diff check to inside skip_row() which might be a bit cleaner.

[manelclos]

I now see what you mean, thanks.

So the problem is that setting skip_unchanged to True is not compatible with settings skip_diff to True because we need original to diff it, which is not what you want because you asked to skip diffs operations.

I'd say that we should check about this situation early, like in the __init__ method and fail with a descriptive message when both options are set to True.

It would be good to know what you and others think about this solution.

[matthewhegarty]

I can add an __init__ check if required.

However, I think since users have to opt in to skip_unchanged, I think it is ok to add a check in skip_row():

        if not self._meta.skip_unchanged or self._meta.skip_diff:
            return False

(I'll then remove the other check)

I will also document in skip_row() why this check is present and why the two flags are not compatible.

The other option is to always set original, but I don't like this because it will remove some of the performance improvement.

Thanks for reviewing!

[manelclos]

I think your proposal for skip_row() is a good solution, developers using both variables set to True should be able to understand the behaviour and identify the solution.

Let's wait for others to comment before you do any modification about this.

Thanks for the PR, I think it is a good first step into optimising import speed.

[matthewhegarty]

I've updated the PR as discussed and added an extra test.

[matthewhegarty]

@manelclos You kindly reviewed this at the end of last year. Please can we get a decision on merging to master?

[andrewgy8]

LGTM. I see no issue with merging this

[matthewhegarty]

Thanks @andrewgy8 and @manelclos

[andrewgy8]

Thank you!