Mutable" no longer being used #727
Conversation
salman2013
changed the title
xblock/test/test_fields.py
Outdated
| @@ -703,7 +703,7 @@ def test_setting_the_same_value_marks_field_as_dirty(): | |||
| # pylint: disable=unsubscriptable-object | |||
| class FieldTester(XBlock): | |||
| """Test block for set - get test.""" | |||
| non_mutable = String(scope=Scope.settings) | |||
| # non_mutable = String(scope=Scope.settings) | |||
There was a problem hiding this comment.
Why have you commented them rather than removing them?
There was a problem hiding this comment.
i mistakenly left them, i have removed them.
|
@salman2013 Approved |
| @@ -720,7 +712,6 @@ class Float(JSONField): | |||
| something for which float(value) does not throw an error. | |||
|
|
|||
| """ | |||
| MUTABLE = False | |||
There was a problem hiding this comment.
Hmm, looking at this more closely, there might be a memory usage implication for this change. It may produce more copies of the defaults than we want for certain objects. Perhaps this is something we need to keep after all.
My thought is this: If whenever we ask for the default value of certain fields, it makes a new copy of the default value via deepcopy then we will increase the memory usage of the application each time a new default is loaded. Given that these are fields in xblocks, that might happen quite a bit, so I wonder if it actually makes more sense to not make this change and instead decide that we're happy with the code as is. @salman2013 @farhan what do you guys think?
There was a problem hiding this comment.
@feanil I think your point of increasing memory usage makes sense and we should not make this change, actually when i removed it the perception in my mind was the value MUTABLE is true by default
MUTABLE = True
_default = None
# Indicates if a field's None value should be sent to the XML representation.
none_to_xml = False
@@ -334,10 +333,7 @@ def init(self, help=None, default=UNSET, scope=Scope.content, # pylint:disa
@Property
def default(self):
"""Returns the static value that this defaults to."""
if self.MUTABLE:
return copy.deepcopy(self._default)
and its already making a deep copy and maybe it would be for some reason but yeah your point is valid it would increase app memory size whenever it is used.
There was a problem hiding this comment.
Ok, let's abandon this PR and close the related issue with a link to this PR and a summary of the reason why we did not pursue it.
|
For the reason of the following discussion i am marking this PR as close and we will not make this change merge. |
Ticket Reference: https://github.com/orgs/openedx/projects/55/views/1?pane=issue&itemId=47987024