On červenec 21st, 2014, 8:33 odp. CEST, Miloslav Trmac wrote:

config/roles/testrole/role.py (Diff revision 1)
109	def get_property(x, prop):	107	def do_get_property(x, prop):
110	# At first cover additional settings.	108	# Cover additional settings.
111	# Then return the result of the call to get_property of the
112	# parent class.
113	if hasattr(x, "_settings") and prop in x._settings:
114	return x._settings[prop]
115	if prop == "myownsetting":	109	if prop == "myownsetting":
116	return x._name	110	return x._name

Silently returns None on an unmatched property

On červenec 22nd, 2014, 2:36 odp. CEST, Thomas Woerner wrote:

get_property is not needed if the setting is in _DEFAULTS, see RoleBase.get_property:
    if hasattr(x, "_settings") and prop in x._settings:
        return x._settings[prop]
    ...
    elif prop in x._DEFAULTS:
        return x._DEFAULTS[prop]


Therefore settings in _settings and in _DEFAULTS are already taken care of. This means get_property should not be needed in a Role.

The concern is about completely unknown setting names; the final do_get_property() just returns None for them.

On červenec 21st, 2014, 8:33 odp. CEST, Miloslav Trmac wrote:

src/rolekit/server/rolebase.py (Diff revision 1)
def get_property(x, prop):
		165	except:

Because the method returns None, this doesn’t fire.
Blind “except:” is evil.  Either define a specific exception type for this, or at the very least an unique object: 

  PropertyNotRecognizedValue = object();

globally, then

   v = x.do_get_property(…)

   if v is not PropertyNotRecognizedValue:

     return v

On červenec 21st, 2014, 8:33 odp. CEST, Miloslav Trmac wrote:

(Or is there a guarantee that None can never be a valid property value?  If so, None could be used instead of PropertyNotRecognizedValue.)

On červenec 22nd, 2014, 2:06 odp. CEST, Thomas Woerner wrote:

None is not accepted by dbus.

OK, but the code still doesn’t work as written because it just tries to return None and UNKNOWN_SETTING is never returned.

- Miloslav

On červenec 22nd, 2014, 5:35 odp. CEST, Thomas Woerner wrote:

Simplifies the role interface
Role cleanup by dropping "failonthis" setting