pekka.klarck commented on revision r3094 in project robotframework.
Details are at http://code.google.com/p/robotframework/source/detail?r=3094
Score: Neutral
General Comment:
This looks like a great start.
Did you consider calling the new `SIGNAL.start/stop_running_keyword` inside
`_run_handler_with_output_captured`? That looks pretty natural place for me
and you wouldn't need to wrap the executed handler. The method should
obviously be renamed in that case.
See also my line-by-line comments.
Line-by-line comments:
File: /trunk/src/robot/running/handlers.py (r3094)
===============================================================================
Line 103: def _signal_wrapper(runnable):
-------------------------------------------------------------------------------
This wrapper is not needed. You could (and should) just use `return
_wrapped_function` instead. Python has nested scopes:
http://docs.python.org/glossary.html?highlight=nested%20scopes#term-nested-scope
Line 109: ROBOT_SIGNAL_HANDLER.stop_running_keyword()
-------------------------------------------------------------------------------
This looks like an excellent place to use context managers instead of
try/finally:
http://docs.python.org/reference/datamodel.html#context-managers
File: /trunk/src/robot/utils/signalhandler.py (r3094)
===============================================================================
Line 21: class _RobotSignalHandler(object):
-------------------------------------------------------------------------------
Why the `Robot` prefix? Could this class be called `StopSignalMonitor`?
Line 32: sys.__stderr__.write("Stopping execution. Second signal
will force exit.")
-------------------------------------------------------------------------------
Having "gracefully" in the message would be good.
Line 52: return self.count > 0
-------------------------------------------------------------------------------
Looks like dead code.
Line 55: ROBOT_SIGNAL_HANDLER = _RobotSignalHandler()
-------------------------------------------------------------------------------
Also here ROBOT prefix isn't really needed. We don't name classes/attrs
with that prefix internally in general.
Respond to these comments at
http://code.google.com/p/robotframework/source/detail?r=3094
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--
Subscription settings:
http://groups.google.com/group/robotframework-commit/subscribe?hl=en
