----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review57 -----------------------------------------------------------
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 http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment43
Silently returns None on an unmatched property
config/roles/testrole/role.py http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment44
Silently returns None on an unmatched property
src/rolekit/server/rolebase.py http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment45
1. Because the method returns None, this doesn’t fire. 2. 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 http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment46
1. Because the method returns None, this doesn’t fire. 2. 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 Čec. 21, 2014, 3:16 odp., Thomas Woerner wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/
(Updated Čec. 21, 2014, 3:16 odp.)
Review request for RoleKit Mailing List, Stephen Gallagher and Thomas Woerner.
Repository: rolekit
Description
Simplifies the role interface Role cleanup by dropping "failonthis" setting
Diffs
config/roles/testrole/role.py 2f077c62b4a8027e7783a2e08c84bc9c9715393e src/rolekit/server/rolebase.py 50b5685a038789d02d3f3b0451f5edaecc187964
Diff: http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/
Testing
Thanks,
Thomas Woerner