| This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/ |
We do need an automated test suite sooner rather than later… these kinds of oversights should be caught by code, not by manual review.
| 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
| config/roles/testrole/role.py (Diff revision 1) | |||
|---|---|---|---|
| 132 | def get_dbus_property(x, prop): |
122 | def do_get_dbus_property(x, prop): |
| 133 | # At first cover additional settings and return a proper dbus type. |
123 | # Cover additional settings and return a proper dbus type. |
| 134 | # Then return the result of the call to get_dbus_property of the |
||
| 135 | # parent class. |
||
| 136 | if prop == "myownsetting": |
124 | if prop == "myownsetting": |
| 137 | return dbus.String(x.get_property(x, prop)) |
125 | return dbus.String(x.get_property(x, prop)) |
Silently returns None on an unmatched property
| 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
| src/rolekit/server/rolebase.py (Diff revision 1) | |||
|---|---|---|---|
def get_dbus_property(x, prop): |
|||
| 194 | 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
- Miloslav Trmac
On červenec 21st, 2014, 3:16 odp. CEST, Thomas Woerner wrote:
|
Review request for RoleKit Mailing List, Stephen Gallagher and Thomas Woerner.
By Thomas Woerner.
Updated Čec. 21, 2014, 3:16 odp.
Repository:
rolekit
Description
Diffs
|