Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable" no longer being used #727

Closed
wants to merge 4 commits into from
Closed

Mutable" no longer being used #727

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3698982
chore: remove mutable property
salman2013 Mar 15, 2024
c756801
chore: fix tests
salman2013 Mar 15, 2024
1ce80fa
chore: remove commented code
salman2013 Mar 15, 2024
0acb60c
Merge branch 'master' into salman/remove-mutable-property
salman2013 Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 2 additions & 13 deletions xblock/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ class will want to refer to.
runtime_options.

"""
MUTABLE = True
_default = None
# Indicates if a field's None value should be sent to the XML representation.
none_to_xml = False
Expand All @@ -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)
else:
return self._default
return copy.deepcopy(self._default)

@property
def name(self):
Expand Down Expand Up @@ -516,10 +512,7 @@ def __get__(self, xblock, xblock_class):
value = self.default
self._set_cached_value(xblock, value)

# If this is a mutable type, mark it as dirty, since mutations can occur without an
# explicit call to __set__ (but they do require a call to __get__)
if self.MUTABLE:
self._mark_dirty(xblock, value)
self._mark_dirty(xblock, value)

return self._sanitize(value)

Expand Down Expand Up @@ -701,7 +694,6 @@ class Integer(JSONField):
containing a floating point number ('3.48') will throw an error.

"""
MUTABLE = False

def from_json(self, value):
if value is None or value == '':
Expand All @@ -720,7 +712,6 @@ class Float(JSONField):
something for which float(value) does not throw an error.

"""
MUTABLE = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def from_json(self, value):
if value is None or value == '':
Expand Down Expand Up @@ -750,7 +741,6 @@ class Boolean(JSONField):
None - > False

"""
MUTABLE = False

# We're OK redefining built-in `help`
def __init__(self, help=None, default=UNSET, scope=Scope.content, display_name=None,
Expand Down Expand Up @@ -853,7 +843,6 @@ class String(JSONField):
The value, as loaded or enforced, can either be None or a basestring instance.

"""
MUTABLE = False
BAD_REGEX = re.compile('[\x00-\x08\x0b\x0c\x0e-\x1f\ud800-\udfff\ufffe\uffff]', flags=re.UNICODE)

def _sanitize(self, value):
Expand Down
1 change: 0 additions & 1 deletion xblock/reference/plugins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ class Filesystem(Field):
src=...> will typically be faster through this than serving that
up through XBlocks views.
"""
MUTABLE = False

def __get__(self, xblock, xblock_class):
"""
Expand Down
10 changes: 5 additions & 5 deletions xblock/test/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you commented them rather than removing them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mistakenly left them, i have removed them.

list_field = List(scope=Scope.settings)
dict_field = Dict(scope=Scope.settings)

Expand All @@ -714,17 +714,17 @@ class FieldTester(XBlock):
assert len(field_tester._dirty_fields) == 0
assert not field_tester.fields['list_field'].is_set_on(field_tester)
assert not field_tester.fields['dict_field'].is_set_on(field_tester)
assert not field_tester.fields['non_mutable'].is_set_on(field_tester)
# assert not field_tester.fields['non_mutable'].is_set_on(field_tester)

field_tester.non_mutable = field_tester.non_mutable
# field_tester.non_mutable = field_tester.non_mutable
field_tester.list_field = field_tester.list_field
field_tester.dict_field = field_tester.dict_field

assert not field_tester.fields['non_mutable'] in field_tester._dirty_fields
# assert not field_tester.fields['non_mutable'] in field_tester._dirty_fields
assert field_tester.fields['list_field'] in field_tester._dirty_fields
assert field_tester.fields['dict_field'] in field_tester._dirty_fields

assert not field_tester.fields['non_mutable'].is_set_on(field_tester)
# assert not field_tester.fields['non_mutable'].is_set_on(field_tester)
assert not field_tester.fields['list_field'].is_set_on(field_tester)
assert not field_tester.fields['dict_field'].is_set_on(field_tester)

Expand Down
Loading