Conversation
…mand. - Minor improvement of an error message.
There was a problem hiding this comment.
Instead of "extra_options", I think "instrumentation_options" is a better name for the feature.
|
I'm on the fence as to whether this is a good idea to include. Generally, I prefer to keep tools as simple as possible. For commandr that means that any functionality that could just as easily be implemented in the wrapped functions themselves (or as a separate decorator) should go there, and not hidden in commandr's internals. That said, this is certainly related to command execution, and would be a bit tricky to implement cleanly. |
|
Thanks for the feedback. I agree with all the coding style comments. I think the name instrumentation_options is good. The reason I called it extra_options is that I was anticipating other types of "hidden" options in the future if this change is accepted :) Regarding the last comment - I think the advantage of building the timeit functionality as an option is that the instrumentation can be switched on/off at run-time (once instrumentation_option is enabled in the run function). If the functionality is implemented inside the code as wrapped functions or decorator, the code needs to change to switch on instrumentation. The code change might be trivial, but if the script is in a production environment and one wants to do some quick benchmarking, one may not want to make temporary changes to a script. |
|
I don't think this functionality should be on by default -- instrumentation is useful for a subset of applications, but most will never make use of them. Also, the 'timeit' module has side-effects (like disabling the GC) that you really don't want to be generally enabled. I think we will be shortly adding a plugin style system to extend commandr without having to be integrated directly into the core. I think this would be a good use case for that system. |
Added a new timeit feature to measure the execution time of a command.
Added a new extra_options parameter to the Run command. The timeit feature is only available if extra_options is set to true. extra_options is False by default.
An example and explanation of the timeit feature can be found in the example_extra.py file
I chose to hide the timeit option by default to avoid clouding the real command options list for users who don't need it. In the future other extra options can be added - some ideas include repeating the command x number of times to do benchmarking, and do a random sleep before running a command to avoid server cron jobs from running the command at the exact same time.
I welcome feedback on whether these features are useful, and whether there are some better ways to organize them. Thanks