AndrewIngram / django-extra-views

Django's class-based generic views are awesome, let's have more of them.
MIT License
1.39k stars 172 forks source link

Behavior of immutable attributes #237

Closed CleitonDeLima closed 3 years ago

CleitonDeLima commented 3 years ago

When using get_inlines and get_inlines_names, a new copy of the object is returned, avoiding changing the class attribute.

codecov-commenter commented 3 years ago

Codecov Report

Merging #237 (7d11b65) into master (415ef28) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   88.77%   88.79%   +0.02%     
==========================================
  Files           6        6              
  Lines         490      491       +1     
  Branches       54       54              
==========================================
+ Hits          435      436       +1     
  Misses         40       40              
  Partials       15       15              
Impacted Files Coverage Δ
extra_views/advanced.py 96.33% <100.00%> (+0.03%) :arrow_up:

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 415ef28...7d11b65. Read the comment docs.

jonashaag commented 3 years ago

Is there something similar or identical in Django? Is copying a common approach in the Django codebase?

sdolemelipone commented 3 years ago

When mutable class level attributes are returned by instance methods they are generally copied, e.g. see FormMixin.get_initial() here: https://github.com/django/django/blob/main/django/views/generic/edit.py

This commit differs from Django in that it is using deepcopy() rather than a shallow .copy() or [:].

jonashaag commented 3 years ago

Thank you! Shouldn’t we use copy as well? We don’t want to make copies of the classes, only return a fresh dict.

sdolemelipone commented 3 years ago

Yes, I think you're right! I like how asking the right question gets the right answer :-)

@CleitonDeLima would you re-write this to use [:] instead of deepcopy? Or can you suggest why deepcopy would be preferred over a shallow copy?

CleitonDeLima commented 3 years ago

Yes, I think you're right! I like how asking the right question gets the right answer :-)

@CleitonDeLima would you re-write this to use [:] instead of deepcopy? Or can you suggest why deepcopy would be preferred over a shallow copy?

Hi @sdolemelipone , It really could just be the copy, I can change that.
I didn't use [:] because tuples can be used.

a = (1, 2, 3)
id(a) == id(a[:])  # is True
sdolemelipone commented 3 years ago

Thanks! LGTM.

Just as a side discussion, as tuples are immutable, does the [:] matter? I think we are only protecting against mutable lists being modified via the instance.

CleitonDeLima commented 3 years ago

Thanks! LGTM.

Just as a side discussion, as tuples are immutable, does the [:] matter? I think we are only protecting against mutable lists being modified via the instance.

Of course! I'm getting in my way on basic concepts, treat the problem with [:] is enough. 😅 Thanks!

sdolemelipone commented 3 years ago

Even better! :smile: Thanks, I hadn't thought about what happens when you call [:] on a tuple until now.

jonashaag commented 3 years ago

I'd have preferred the explicit copy way personally but this is fine. Thanks everyone!

sdolemelipone commented 3 years ago

I'd have preferred the explicit copy way personally but this is fine. Thanks everyone!

Interesting! Why, because explicit is better than implicit? For me [:] appeared more beautiful/elegant. Also, I didn't know that:

from copy import copy
a = (1, 2, 3)
id(a) == id(copy(a))  # is True
jonashaag commented 3 years ago

Yes, and because it’s done that way in the Django codebase. Needlessly doing things differently in one place than in the other increases chances of creating bugs, adds to the mental load of readers, etc.