Make None default for exclude fields

[saschahofmann]

Closes #549
@jackton1 That's all I had in mind. I had a quick scan over get_fields_and_unique_fields_from_cls which seems to be the only other place the excluded fields are used but I don't see anything obvious where this could break things.

[github-actions]

Thanks for implementing a fix. Could you include a test that covers your changes.

[codecov]

Codecov Report

Merging #551 (fd29e51) into main (a82b657) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #551   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          285       285           
=========================================
  Hits           285       285           

Impacted Files	Coverage Δ
model_clone/mixin.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a82b657...fd29e51. Read the comment docs.

[jackton1]

@saschahofmann other than changing the default type ([] -> None) are there any cases where the excluded fields would result in skipping fields?

Arguably the use of any should evaluate to True even if the field isn’t excluded.

[saschahofmann]

Not sure I understand the question? The point of changing the behavior is that I expected all fields to be included when I provide an empty list for excluded fields. Maybe there are more explicit solutions to achieve this (e.g. another flag) but I thought this would already be quite good.

Arguably the use of any should evaluate to True even if the field isn’t excluded.

I didnt change the behaviour of the any clause except that now if you provide an empty list the clause will actually return True . If you don't change the exclude default but use an include the first part of any can of course still return True?

Sorry if I am overexplaining not sure I got it 😅

[jackton1]

@saschahofmann The design of the CloneMixin is to only copy over all local model fields by default and require explicit list of fields in the case of a related field i.e o2o, m2m etc.

One way to get around copying all fields would be to provide an exclusion list with one field i.e all except this field.

If your looking to get around declaring an excluded or included list of fields this change wouldn’t support copying related data from non declared fields since we also check for the field name existing in list.

[saschahofmann]

I think I'm not trying to get around declaring a list of excluded fields. This change has two effects:

1. it allows for a method to clone all fields of a model (by providing an empty list as the excluded fields) . !

2. I think it's the more logical behaviour. I think the default should be as you have it right now. If the developer doesn't provide any lists (include or exclude) none of the related fields are copied. This behaviour is maintained with the current changes.

But if I provide a list with one field suddenly all fields except that field are copied. Ok that makes sense but if I provide an empty list I think the logical behaviour would be that all fields are cloned!

PS: I'm actually going to comment on the other issue I have open with some more suggestions on how copying all fields could be made more robust.

[saschahofmann]

And I think your last point isn't true? If I now provide an empty list for an exclude the first part of the any will still return false but second part now returns true.

That being said maybe I should write a test for that.

[jackton1]

@saschahofmann After taking a look seems your right using an empty list should get around the issue.

Though I’m still thinking that there should be a better way instead of having a user declare an empty list to get the behaviour of copying all related fields.

A separate class property can be used toggle on the copy all related fields functionality as opposed to an empty list and we can always assume the excluded fields to be a list and check for the presence or absence of the field.

[saschahofmann]

I think it'd definitely be an option to create another flag but do you disagree that the naive expectation of providing an empty list (not knowing that it's the default already) would be that it includes are all fields?

[jackton1]

Since the presence of a list would create two side effects ie. (Don’t copy field ‘x’ in the list, but also copy all fields if the list is empty). I’ll prefer using a more explicit flag to distinguish between the later.

[saschahofmann]

Interesting, for me that is a single side-effect 😄 and exactly what I expected to happen but I guess we have to agree to disagree here then.