Commit 76f8f4b8 authored by Julien Muchembled's avatar Julien Muchembled

* Fix several bugs causing:

  * _set.* accessors to reindex the object
  * set.* category accessors to reindex the object several times
* Remove old/unused code in ERP5Type/Accessor
* Some optimisations in ERP5Type.Accessor.List

git-svn-id: https://svn.erp5.org/repos/public/erp5/trunk@26781 20353a03-c40f-0410-a6d1-a30d3c3de9de
parent 6edc4df1
......@@ -590,9 +590,14 @@ class CategoryTool( UniqueObject, Folder, Base ):
return membership
security.declareProtected( Permissions.AccessContentsInformation, 'setCategoryMembership' )
def setCategoryMembership(self, context, base_category_list, category_list, base=0, keep_default=1,
spec=(), filter=None,
checked_permission=None, **kw ):
def setCategoryMembership(self, context, *args, **kw):
self._setCategoryMembership(context, *args, **kw)
context.reindexObject()
def _setCategoryMembership(self, context, base_category_list,
category_list, base=0, keep_default=1,
spec=(), filter=None, checked_permission=None,
**kw):
"""
Sets the membership of the context on the specified base_category
list and for the specified portal_type spec
......@@ -701,7 +706,7 @@ class CategoryTool( UniqueObject, Folder, Base ):
# 'new_category_list: %s' % str(new_category_list))
# LOG("CategoryTool, setCategoryMembership", 0 ,
# 'default_new_category_list: %s' % str(default_new_category_list))
self.setCategoryList(context, tuple(default_new_category_list + new_category_list))
self._setCategoryList(context, tuple(default_new_category_list + new_category_list))
security.declareProtected( Permissions.AccessContentsInformation, 'setDefaultCategoryMembership' )
......
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from Base import func_code, type_definition, list_types, ATTRIBUTE_PREFIX, Getter as BaseGetter, Setter as BaseSetter
from Products.ERP5Type.PsycoWrapper import psyco
......@@ -134,7 +133,6 @@ class Setter(BaseSetter):
acquisition_object_id = None,
is_list_type = 0,
is_tales_type = 0,
reindex = 0
):
if type(portal_type) == type('a'): portal_type = (portal_type, )
self._id = id
......@@ -157,7 +155,6 @@ class Setter(BaseSetter):
self._acquisition_object_id = acquisition_object_id
self._is_list_type = is_list_type
self._is_tales_type = is_tales_type
self._reindex = reindex
def __call__(self, instance, value, *args, **kw):
from Products.ERP5Type.Utils import assertAttributePortalType
......@@ -166,13 +163,7 @@ class Setter(BaseSetter):
if o is None:
o = instance.newContent(id=self._storage_id,
portal_type=self._portal_type[0])
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
o.setProperty(self._acquired_property, value, *args, **kw)
else:
o._setProperty(self._acquired_property, value, *args, **kw)
o._setProperty(self._acquired_property, value, *args, **kw)
return (o, )
DefaultSetter = Setter
......
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from ZPublisher.HTTPRequest import FileUpload
from TypeDefinition import type_definition, list_types, ATTRIBUTE_PREFIX
......@@ -56,11 +55,10 @@ class Setter(Method):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, property_type, storage_id=None, reindex=1):
def __init__(self, id, key, property_type, storage_id=None):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
self._property_type = property_type
self._cast = type_definition[property_type]['cast']
self._null = type_definition[property_type]['null']
......@@ -71,41 +69,31 @@ class Setter(Method):
def __call__(self, instance, *args, **kw):
value = args[0]
modified_object_list = []
if not self._reindex:
# Modify the property
if value in self._null:
setattr(instance, self._storage_id, None)
elif self._property_type == 'content':
# A file object should be provided
file_upload = args[0]
if isinstance(file_upload, FileUpload):
if file_upload:
try:
o = getattr(instance, self._storage_id)
except AttributeError:
# We create a default type
o = instance.PUT_factory(self._storage_id,
file_upload.headers.get('content-type', None), file_upload )
instance._setObject(self._storage_id, o)
o = getattr(instance, self._storage_id)
o.manage_upload(file = file_upload)
modified_object_list = [o]
else:
LOG('ERP5Type WARNING',0,'%s is not an instance of FileUpload' % str(file_upload))
# Modify the property
if value in self._null:
setattr(instance, self._storage_id, None)
elif self._property_type == 'content':
# A file object should be provided
file_upload = args[0]
if isinstance(file_upload, FileUpload):
if file_upload:
try:
o = getattr(instance, self._storage_id)
except AttributeError:
# We create a default type
o = instance.PUT_factory(self._storage_id,
file_upload.headers.get('content-type', None), file_upload)
instance._setObject(self._storage_id, o)
o = getattr(instance, self._storage_id)
o.manage_upload(file = file_upload)
modified_object_list = [o]
else:
setattr(instance, self._storage_id, self._cast(args[0]))
modified_object_list.append(instance)
return modified_object_list
LOG('ERP5Type WARNING', 0, '%s is not an instance of FileUpload'
% str(file_upload))
else:
# Call the private setter
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
method = getattr(instance, '_' + self._id)
method(*args, **kw)
instance.reindexObject() # XXX Should the Setter check
# if the property value has changed
# to decide reindexing, like edit() ?
setattr(instance, self._storage_id, self._cast(args[0]))
modified_object_list.append(instance)
return modified_object_list
from Products.CMFCore.Expression import Expression
def _evaluateTales(instance=None, value=None):
......
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from Base import func_code, type_definition, list_types, ATTRIBUTE_PREFIX, Setter as BaseSetter, Getter as BaseGetter
from zLOG import LOG
......@@ -45,11 +44,10 @@ class ListSetter(BaseSetter):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, reindex=1):
def __init__(self, id, key):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
def __call__(self, instance, *args, **kw):
instance._setCategoryMembership(self._key, args[0],
......@@ -59,11 +57,6 @@ class ListSetter(BaseSetter):
base=kw.get('base', 0),
keep_default=0,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
Setter = ListSetter
......@@ -81,11 +74,10 @@ class DefaultSetter(BaseSetter):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, reindex=1):
def __init__(self, id, key):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
def __call__(self, instance, *args, **kw):
instance._setDefaultCategoryMembership(self._key, args[0],
......@@ -94,11 +86,6 @@ class DefaultSetter(BaseSetter):
portal_type=kw.get('portal_type',()),
base=kw.get('base', 0),
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
class SetSetter(BaseSetter):
......@@ -114,11 +101,10 @@ class SetSetter(BaseSetter):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, reindex=1):
def __init__(self, id, key):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
def __call__(self, instance, *args, **kw):
"""
......@@ -139,11 +125,6 @@ class SetSetter(BaseSetter):
base=kw.get('base', 0),
keep_default=1,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
......
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from Base import func_code, type_definition, ATTRIBUTE_PREFIX, Method
import Base
......@@ -188,7 +187,7 @@ class Setter(Base.Setter):
func_defaults = ()
def __init__(self, id, key, property_type, acquired_property,
portal_type=None, storage_id=None, reindex=0):
portal_type=None, storage_id=None):
self._id = id
self.__name__ = id
self._key = key
......@@ -203,7 +202,6 @@ class Setter(Base.Setter):
portal_type = (portal_type,)
self._portal_type = portal_type
self._acquired_property = acquired_property
self._reindex = reindex
def __call__(self, instance, *args, **kw):
# We return the first available object in the list
......@@ -214,26 +212,14 @@ class Setter(Base.Setter):
o = instance._getOb(k, None)
if o is None: available_id = k
if o is not None and o.portal_type in self._portal_type:
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
o.setProperty(self._acquired_property, *args, **kw)
else:
o._setProperty(self._acquired_property, *args, **kw)
o._setProperty(self._acquired_property, *args, **kw)
modified_object_list = (o, )
if o is None and available_id is not None:
from Products.ERP5Type.Utils import assertAttributePortalType
assertAttributePortalType(instance, available_id, self._portal_type)
o = instance.newContent(id=available_id,
portal_type=self._portal_type[0])
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
o.setProperty(self._acquired_property, *args, **kw)
else:
o._setProperty(self._acquired_property, *args, **kw)
o._setProperty(self._acquired_property, *args, **kw)
modified_object_list = (o, )
return modified_object_list
......
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from Base import func_code, type_definition, list_types,\
ATTRIBUTE_PREFIX, Method, evaluateTales
......@@ -52,11 +51,10 @@ class DefaultSetter(Base.Setter):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, property_type, storage_id=None, reindex=1):
def __init__(self, id, key, property_type, storage_id=None):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
self._property_type = property_type
if property_type in list_types: # classic list
self._cast = type_definition[property_type]['cast']
......@@ -73,56 +71,33 @@ class DefaultSetter(Base.Setter):
def __call__(self, instance, *args, **kw):
# Turn the value into a list
value = args[0]
if not self._reindex:
# Modify the property
if self._is_tales_type:
setattr(instance, self._storage_id, str(value))
elif value in self._null:
# The value has no default property -> it is empty
setattr(instance, self._storage_id, ())
else:
if self._item_cast is not identity:
value = self._item_cast(value)
default_value = value
list_value = getattr(instance, self._storage_id, None)
if list_value is None: list_value = []
my_dict = dict([(x, 0) for x in list_value if x != default_value])
new_list_value = my_dict.keys()
new_list_value.insert(0, default_value)
value = new_list_value
setattr(instance, self._storage_id, tuple(value))
# Modify the property
if self._is_tales_type:
setattr(instance, self._storage_id, str(value))
elif value in self._null:
# The value has no default property -> it is empty
setattr(instance, self._storage_id, ())
else:
# Call the private setter
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
method = getattr(instance, '_' + self._id)
method(*args, **kw)
instance.reindexObject()
if self._item_cast is not identity:
value = self._item_cast(value)
list_value = set(getattr(instance, self._storage_id, ()))
list_value.discard(value)
setattr(instance, self._storage_id, (value,) + tuple(list_value))
class ListSetter(DefaultSetter):
def __call__(self, instance, *args, **kw):
value = args[0]
if not self._reindex:
# Modify the property
if value in self._null:
setattr(instance, self._storage_id, ())
elif self._is_tales_type:
setattr(instance, self._storage_id, str(value))
else:
value = self._cast(args[0])
if self._item_cast is not identity:
value = map(self._item_cast, value)
setattr(instance, self._storage_id, tuple(value))
# Modify the property
if value in self._null:
setattr(instance, self._storage_id, ())
elif self._is_tales_type:
setattr(instance, self._storage_id, str(value))
else:
# Call the private setter
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
method = getattr(instance, '_' + self._id)
method(*args, **kw)
instance.reindexObject()
value = self._cast(args[0])
if self._item_cast is not identity:
value = map(self._item_cast, value)
setattr(instance, self._storage_id, tuple(value))
Setter = ListSetter
......@@ -140,11 +115,10 @@ class SetSetter(Base.Setter):
func_code.co_argcount = 2
func_defaults = ()
def __init__(self, id, key, property_type, storage_id=None, reindex=1):
def __init__(self, id, key, property_type, storage_id=None):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
self._property_type = property_type
if property_type in list_types: # classic list
self._cast = type_definition[property_type]['cast']
......@@ -161,42 +135,27 @@ class SetSetter(Base.Setter):
def __call__(self, instance, *args, **kw):
# Turn the value into a list
value = args[0]
if not self._reindex:
# Modify the property
if self._is_tales_type:
setattr(instance, self._storage_id, str(value))
elif value in self._null:
setattr(instance, self._storage_id, ())
else:
value = self._cast(args[0])
if self._item_cast is not identity:
value = map(self._item_cast, value)
if len(value) > 0:
list_value = getattr(instance, self._storage_id, None)
if list_value is None: list_value = []
if len(list_value) > 0:
default_value = list_value[0]
my_dict = dict([(x, 0) for x in value if x != default_value])
new_list_value = my_dict.keys()
# Modify the property
if self._is_tales_type:
setattr(instance, self._storage_id, str(value))
elif value in self._null:
setattr(instance, self._storage_id, ())
else:
value = self._cast(value)
if self._item_cast is not identity:
value = map(self._item_cast, value)
if value:
value = set(value)
list_value = getattr(instance, self._storage_id, None)
if list_value:
default_value = list_value[0]
if default_value in value:
# If we change the set,
# the default value must be in the new set
if default_value in value:
new_list_value.insert(0, default_value)
else:
new_list_value = value
else:
# The list has no default property -> it is empty
new_list_value = []
setattr(instance, self._storage_id, tuple(new_list_value))
return (instance, )
else:
# Call the private setter
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
method = getattr(instance, '_' + self._id)
method(*args, **kw)
instance.reindexObject()
value.remove(default_value)
value = (default_value,) + tuple(value)
setattr(instance, self._storage_id, tuple(value))
return (instance, )
class DefaultGetter(Base.Getter):
"""
......@@ -311,7 +270,6 @@ class SetGetter(ListGetter):
def __call__(self, instance, *args, **kw):
result_list = ListGetter.__call__(self, instance, *args, **kw)
if result_list is not None:
result_set = dict([(x, 0) for x in result_list]).keys()
return result_set
return list(set(result_list))
Tester = Base.Tester
......@@ -26,7 +26,6 @@
#
##############################################################################
import warnings
from Base import func_code, type_definition, list_types, ATTRIBUTE_PREFIX, Setter as BaseSetter, Getter as BaseGetter
from zLOG import LOG
......@@ -40,11 +39,10 @@ class SetSetter(BaseSetter):
# This can not be called from the Web
def __init__(self, id, key, reindex=1, warning=0):
def __init__(self, id, key, warning=0):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
self._warning = warning
def __call__(self, instance, *args, **kw):
......@@ -56,11 +54,6 @@ class SetSetter(BaseSetter):
portal_type=kw.get('portal_type',()),
keep_default=1,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
psyco.bind(__call__)
......@@ -80,11 +73,6 @@ class ListSetter(SetSetter):
portal_type=kw.get('portal_type',()),
keep_default=0,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
psyco.bind(__call__)
......@@ -105,11 +93,6 @@ class DefaultSetter(SetSetter):
filter=kw.get('filter', None),
portal_type=kw.get('portal_type',()),
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
psyco.bind(__call__)
......@@ -458,11 +441,10 @@ class UidSetSetter(BaseSetter):
# This can not be called from the Web
def __init__(self, id, key, reindex=1, warning=0):
def __init__(self, id, key, warning=0):
self._id = id
self.__name__ = id
self._key = key
self._reindex = reindex
self._warning = warning
def __call__(self, instance, *args, **kw):
......@@ -474,11 +456,6 @@ class UidSetSetter(BaseSetter):
portal_type=kw.get('portal_type',()),
keep_default=1,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
class UidListSetter(UidSetSetter):
"""
......@@ -495,11 +472,6 @@ class UidListSetter(UidSetSetter):
portal_type=kw.get('portal_type',()),
keep_default=0,
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
UidSetter = UidListSetter
......@@ -518,11 +490,6 @@ class UidDefaultSetter(UidSetSetter):
filter=kw.get('filter', None),
portal_type=kw.get('portal_type',()),
checked_permission=kw.get('checked_permission', None))
if self._reindex:
warnings.warn("The reindexing accessors are deprecated.\n"
"Please use Alias.Reindex instead.",
DeprecationWarning)
instance.reindexObject()
return (instance, )
class DefaultIdGetter(BaseGetter):
......
......@@ -2215,20 +2215,14 @@ class Base( CopyContainer,
self.reindexObject()
# Private accessors for the implementation of categories
def _setCategoryMembership(self, category, node_list, spec=(),
filter=None, portal_type=(), base=0, keep_default=1,
checked_permission=None):
self._getCategoryTool().setCategoryMembership(self, category, node_list,
spec=spec, filter=filter, portal_type=portal_type, base=base,
keep_default=keep_default, checked_permission=checked_permission)
def _setCategoryMembership(self, *args, **kw):
self._getCategoryTool()._setCategoryMembership(self, *args, **kw)
#self.activate().edit() # Do nothing except call workflow method
# XXX This is a problem - it is used to circumvent a lack of edit
security.declareProtected( Permissions.ModifyPortalContent, 'setCategoryMembership' )
def setCategoryMembership(self, category, node_list, spec=(), portal_type=(), base=0, keep_default=1,
checked_permission=None):
self._setCategoryMembership(category,
node_list, spec=spec, filter=filter, portal_type=portal_type, base=base, keep_default=keep_default, checked_permission=checked_permission)
def setCategoryMembership(self, *args, **kw):
self._setCategoryMembership(*args, **kw)
self.reindexObject()
def _setDefaultCategoryMembership(self, category, node_list,
......
......@@ -1784,7 +1784,7 @@ def createDefaultAccessors(property_holder, id, prop = None,
# Create setters for a list property (no reindexing)
# The base accessor sets the list to a singleton
# and allows simulates a simple property
accessor_args = (prop['type'], prop.get('storage_id'), 0)
accessor_args = (prop['type'], prop.get('storage_id'))
# Create setters for a list property
setter_name = '_set' + UpperCase(id)
if not hasattr(property_holder, setter_name):
......@@ -1838,7 +1838,7 @@ def createDefaultAccessors(property_holder, id, prop = None,
# Create setters for a list property (no reindexing)
# The base accessor sets the list to a singleton
# and allows simulates a simple property
accessor_args = (prop['type'], prop.get('storage_id'), 0)
accessor_args = (prop['type'], prop.get('storage_id'))
# Create setters for an object property
setter_name = '_set' + UpperCase(id)
if not hasattr(property_holder, setter_name):
......@@ -1865,7 +1865,7 @@ def createDefaultAccessors(property_holder, id, prop = None,
if not hasattr(property_holder, setter_name):
property_holder.registerAccessor(setter_name, id, Content.DefaultSetter, accessor_args)
else:
accessor_args = (prop['type'], prop.get('storage_id'), 0)
accessor_args = (prop['type'], prop.get('storage_id'))
# Create setters for a simple property
setter_name = 'set' + UpperCase(id)
if not hasattr(property_holder, setter_name):
......@@ -2000,7 +2000,7 @@ def createCategoryAccessors(property_holder, id,
property_holder.registerAccessor(setter_name, '_' + setter_name, Alias.Reindex, ())
property_holder.declareProtected(write_permission, setter_name)
accessor_args = (0,)
accessor_args = ()
setter_name = '_set' + UpperCase(id)
if not hasattr(property_holder, setter_name):
property_holder.registerAccessor(setter_name, id, Category.Setter, accessor_args)
......@@ -2336,7 +2336,7 @@ def createValueAccessors(property_holder, id,
property_holder.registerAccessor(setter_name, '_' + setter_name, Alias.Reindex, ())
property_holder.declareProtected(write_permission, setter_name)
accessor_args = (0,)
accessor_args = ()
setter_name = '_set' + UpperCase(id) + 'Value'
if not hasattr(property_holder, setter_name):
property_holder.registerAccessor(setter_name, id, Value.Setter, accessor_args)
......
......@@ -442,7 +442,16 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
checkRelationSet(self)
person_object.setRegionValue(None)
checkRelationUnset(self)
# Test _setRegion doesn't reindex the object.
person_object._setRegion(category_id)
get_transaction().commit()
self.assertFalse(person_object.hasActivity())
person_object.setRegion(None)
get_transaction().commit()
self.assertTrue(person_object.hasActivity())
self.tic()
def test_05_setProperty(self, quiet=quiet, run=run_all_test):
"""
In this test we create a subobject (ie. a phone number)
......@@ -805,15 +814,18 @@ class TestPropertySheet:
person = module.newContent(portal_type='Person')
# Do the same tests as in test_11_valueAccessor
person.setSubject('alpha')
self.assertEquals(person.getSubject(), 'alpha')
person.setSubject('beta')
self.assertEquals(person.getSubject(), 'beta')
person.setSubjectList(['alpha', 'alpha'])
self.assertEquals(person.getSubjectList(), ['alpha', 'alpha'])
person.setSubjectSet(['alpha', 'alpha'])
self.assertEquals(person.getSubjectSet(), ['alpha'])
person.setSubjectList(['alpha', 'beta', 'alpha'])
self.assertEquals(person.getSubjectList(), ['alpha', 'beta', 'alpha'])
person.setSubjectSet(['beta', 'beta'])
self.assertEquals(person.getSubjectList(), ['beta'])
self.assertEquals(person.getSubjectSet(), ['beta'])
person.setSubjectList(['beta', 'alpha', 'beta'])
self.assertEquals(person.getSubjectList(), ['beta', 'alpha', 'beta'])
person.setSubjectSet(['alpha', 'beta', 'alpha'])
self.assertEquals(person.getSubjectList(), ['beta', 'alpha'])
result = person.getSubjectSet()
result.sort()
self.assertEquals(result, ['alpha', 'beta'])
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment