-
-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add feature to specify task parameters in doit.cfg (issue #283) #322
Conversation
I'm working on some test cases but I wanted to get this preliminary implementation out for review. Let me know what you think, please. I've not tried watch or auto... probably doesn't work for those commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR that would bring a nice feature.
Apart from tests missing (mandatory :) ) and a name choice that is maybe not the most intuitive (see comment), otherwise looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about requiring a task:
prefix in the config section name. i.e.
instead of
[arg_from_cfg]
write
[task:arg_from_cfg]
This way we also avoid potential conflicts with commands.
doit/cmd_base.py
Outdated
@@ -578,6 +578,10 @@ def execute(self, params, args): | |||
if not legacy_loader: | |||
self.task_list = self.loader.load_tasks(cmd=self, pos_args=args) | |||
|
|||
# Add task options from config, if present | |||
for task in self.task_list: | |||
task.cfg_defaults = self.config.get(task.name, {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It bothers me a bit that we might be calling config.get
for every task when rarely it will contain a value...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is roughly 30% faster to do an if task.name in self.config: ...
. What the actual execution time benefit would be depends on how many tasks are defined.
Benchmark:
https://gist.github.com/rbdixon/797d0478f9716c4123e401a1187552e1
Number of tasks: 1326
Speedup -30.40%
Baseline duration: 0.25ms
Alternative duration: 0.18ms
Might be hard for a user to tell the difference in execution time either way.
This is very helpful and I'll rework the patch. I really appreciate the thoughtful review. |
All command line parsing should be in Task now.
Task.init_options() will only initialize options once. The tests were written so that each test reused the same task instances and therefore the tests failed because the expected task option defaults were not applied. The fix is to use a fixture to redefine the tasks each time. The only-once semantics of Task.init_options() dates back four years and the commit indicates this was a performance enhancement.
I've implemented tests, documentation, and all changes discussed in this thread. I'm using this branch in my daily work with our own internal tools and nikola. So far, so good. I believe this is ready to be merged. |
This is a great feature, just ran into this and want to say thank you and bump it 😃 |
merged: f0303f2 thanks. and sorry for delay. |
doit.cfg options take precedence over task definition default values
but with be replaced by task parameters passed on the command line.
TODO:
[task:arg_from_cfg]
cfg_defaults
tocfg_values
cfg_values
parameter.overwrite_defaults()
to applycfg_values
control.py
per commentImplements #283
Demo: