On Mon, Feb 14, 2011 at 7:46 AM, Michael Gliwinski
<address@hidden> wrote:
On a general note though, wouldn't it be easier (for reviewers and for
merging) if these things were separated into feature branches? The different
features there don't seem to depend on each other anyways.
Agreed -- this started out as a simple fix or two, then ballooned. There are a few pieces to this that can be extracted, but for the most part the __all__, namespace, and @task code depends on refactoring done along the way so extracting it would be rather hard.
Regarding overall approach, what made you choose the Task class as a way to
designate tasks? I'm asking because it doesn't appear to do anything special
and has a disadvantage of replacing the original callable and changing its
signature.
The reason to move to the Task was to get a nice flex point that allows other developers to provide custom types of tasks. Think @depends that returns a class that extends Task (or WrappedCallableTask) and has a custom run method for checking that other tasks have run.
The second reason has not yet started -- moving the actual task setup/execution out of fabric.main into Task.run. Currently if I call another task function inside a task it executes the function, which may or may not be what Fabric does.
Finally, classes provide a nice hook to find out whether a possible task is using Task. Since we want this to be BC, the idea is to only switch to @task decorators if one is present.
All of this could be done via straight decorators and variables set on those decorators. It's 6 of one to a half dozen of another.
I'm just thinking aloud here but it seems to me a registry approach may be
simplier. Fabric already has a sort of task registry (the tasks dict used in
load_fabfile) so it would be a matter of making it global and changing how
it's populated.
By registry here I mean either a simple dict as is already used or something
more fancy like e.g. Registry implementation in bzrlib [1] which allows
storing both objects (or indirect references) and metadata.
I agree. I think the the registry needs to be expanded a little bit. Providing a registry.find_task(name='foo') and the ability to check to see if a task has run (and how many times) are something that definitely need to happen. My original thought for @task was that it would register tasks directly with a registry. Given the current implementation, however, I opted for the scanning of a variable to see if it was a task since it is currently a registry of "sorts". :-)
I like the namespacing implementation, although because "explicit is better
than implicit" I agree with Jeff's comment to #56 [2] about having a
register_subtasks or similar function.
Let me think on this one. My initial inclination leans slightly toward putting it on the task designer to specify what can be used as a task and what can't, but I can definitely see the point of it feeling a bit magic. My reasoning for putting it in the module itself, rather than the fabfile is that the module gets written once, the fabfiles get written multiple times. It seems reasonable to write it once, rather than having to explicitly register it each time.
One point, the register_subtasks code suffers from the same problem that lead to scanning for @task's rather than having @task actually register -- we currently scan the file for tasks rather than having an explicit registry to stuff things into. Now, it could be that we import the fabfile, have @task / register_module_tasks mark their variables as already_processed or some such, but that's a slightly bigger change than what's in the code right now.
-T