task "actions" as a list of commands/python

[schettino72]

aka task & actions de-coupling.

[Javier Collado (javier.collado)]

Hello,

I fixed the issues you pointed out through e-mail (yes, it's better to follow discussion here) in rev.135.

Best regards,
    Javier

P.S. Don't worry about your comments, all of them are welcome. It's ok to stick to the style of the original project code and nice to learn new things such as the white space highlighting configuration in emacs.

[schettino72 (schettino72)]

great.

there is a small bug that InvalidTask is not raised if there is no
'action' or 'actions'. diff below.

unit tests?

=== modified file 'lib/doit/task.py'
--- lib/doit/task.py 2009-09-02 17:20:03 +0000
+++ lib/doit/task.py 2009-09-02 23:25:21 +0000
@@ -286,17 +286,18 @@
     TASK_ATTRS = ('name','actions','dependencies','targets','setup')
     # FIXME check field 'name'

+ # deprecation warning
+ if 'action' in task_dict:
+ task_dict['actions'] = task_dict['action']
+ del task_dict['action']
+ print ("DEPRECATION WARNING: Task %s contains 'action' key. "
+ "This will be removed in future versions, "
+ "please use 'actions' instead" % task_dict['name'])
+
     # check required fields
     if 'actions' not in task_dict:
- if 'action' in task_dict:
- task_dict['actions'] = task_dict['action']
- del task_dict['action']
- print ("WARNING Task %s contains 'action' key. "
- "This will be deprecated in future versions, "
- "please use 'actions' instead" % task_dict['name'])
- else:
- raise InvalidTask("Task %s must contain 'actions' field. %s" %
- (task_dict['name'],task_dict))
+ raise InvalidTask("Task %s must contain 'actions' field. %s" %
+ (task_dict['name'],task_dict))

     # user friendly. dont go ahead with invalid input.
     for key in task_dict.keys():

[Javier Collado (javier.collado)]

Hello,

I tried the code and the exception is raised when no 'action' or 'actions' field is present. Also, an error will be raised when both of them are present (whereas with the suggested change, the 'actions' field will be overwritten). If I skipped something please let me know.

Regarding the unit tests, I'll come back to you when something is ready for review.

Best regards,
    Javier

[schettino72 (schettino72)]

Thats why we should be working with updated unittests all the time.
otherwise we keep going back-and-forth. I didnt think the case where
both "action" and "actions" where used.

Other points:

 * In your revision 139 you break the forget for group tasks. on
main.py I check for "actions is None" but on task.py you do actions =
[]
Both using an empty list or None is ok. If you choose an empty list I
think we will need a variable to indicate when it is a GroupTask. Note
that GroupTasks were used in 2 ways.

1. When a user create a task and set its action to None. and add some
tasks as dependencies
2. when the task define sub-tasks using python generators

When I use "forget" I want also to forget the dependent tasks from
group tasks. but not forget dependent task from other tasks...

* dependencies and targets do not accept single elements it must be a
list. on actions you accept both a single element and a list. this is
inconsistent.
I would prefer to only accept lists for all of them. but if you think
we should support single elements or lists i am ok with that. but they
have to be consistent.

[Javier Collado (javier.collado)]

Hello,

Yes, you're right about the group tasks, I realized about that when running the test cases. The idea was to use the empty list to keep the Task.execute method implementation straightforward so I changed the check in doit_forget so that tasks are removed when self.action is False. Right now (rev. 142) all test cases run perfectly, so I guess that the 2 ways to use GroupTasks are covered. Anyway, I don't feel specially comfortable with the implementation of the Task.__str__ method so maybe some redesign (bring back GroupTask class?) or test case modification is needed. Please take a look a it and let me know your opinion.

Regarding accepting just lists when using the 'actions' key, I agree with you. However, if backwards compatibility is desired (such as accepting 'action' instead of 'actions'), then doit should still accept single parameters and print deprecation warnings. What do you think about this?

Best regards,
    Javier

[schettino72 (schettino72)]

Hi Javier,

 Thanks a lot. I merged your branch into trunk r127 and did some modifications. This way we can move faster. but please also review my changeset and look for mistakes... Below I explain the changes I've done, i hope you agree :)

Notes:
======

* doc variables. You wrote:

    @type action: str
    @ivar action: Command to be passed to the shell subprocess

I know this style is standard but I think it is too verbose. In this project I am using:

   @ivar action(str): Command to be passed to the shell subprocess

* docs ivar and init parameters:

  If you document an ivar no need to repeat its documentation on init method.

* use: #! /usr/bin/env python instead of: #!/usr/bin/python

* Task.__init__ actions should not have a default value on constructor because it is a required field.

* Since "action" is deprecated I expected that after this changeset no test will use "action".

* just fixing the failing test is not enough. you have to add new ones :) I guess you just replace strings because on test_tasks.py you ended-up with many TestTask classes :( they were not even being executed!

* i have a script "unitcoverage". it has to be on 99%. (on util.py and dependency.py) there are a few lines that are executed depending on the python version.

* better try to put deprecated code clearly separated from the rest.

* TODO: python has a "warning" module. I guess it would make it easier to manage and test deprecations...

Regards