Repository navigation
Enhanced @task (aliases etc) #22
Description
Activity
Jeff Forcier (bitprophet) posted:
The following comments were copy/pasted from an experimental Github thread (scroll down a bit if you see a blank space here):
bitprophet
So usage, e.g. aliasing or renaming, would look like this?
@task(kwargs={'name': 'notfoobar'}) def foobar(): passWonder if
def task(func, task_class=None, *args, **kwargs)is legal/a good idea. As a user, I'd much prefer to write this instead of the above:@task(name='notfoobar') def foobar(): pass
tswicegood
Re: aliases... nah, I would add an
alias&aliaseskwarg to the decorator and pass it in that way, but the other would work.Re:
*args,**kwargs, I did try that, but there was an issue withtask_classgetting passed around multiple times. I honestly didn't try playing with it much, so it might be possible. That said, but requiring an explicitkwargsandargskeyword arg it gives us room to treat some of them differently. In other words, there's no "special" **kwargs that don't get passed in (liketask_class).
bitprophet
Re: your last sentence: I understand the implementation motivations of using explicit
args/kwargsparameters, but again, usability should take precedence in my opinion, outside of rare exceptions. So I'd strongly prefer whatever approach lets me stick to regular argument passing when using the decorator (vs passing in data structures).We can always yank out "special" kwargs, that's what Fabric does already with CLI tasks (i.e.
fab mytask:param=value,host=hostAturns intomytask(param=value)and thehost=hostApair gets used to set the host list.) It is not 100% ideal but it's relatively painless, especially if documented.Also, what do you mean by
task_classgetting passed around multiple times?
tswicegood
I was getting this
bitprophet
Can't we just use
task_class = kwargs.pop("task_class")here? :)
tswicegood
Sure could. This is what I get for coding after digging into those
pthfiles yesterday. :-)FYI, not a huge deal, but commit comments are treated differently in pull requests. The diff around the comment isn't pulled in so the context is lost in the stream. We try to keep all of the comments on the diff-view of the PR in Armstrong for that reason.
bitprophet
Thanks, appreciate the change in direction and glad you figured a way around the errors.
bitprophet
Woot, GH pull request auto-convo in action! (re: previous two entries, a commit line note + commit comment respectively)
Just had a thought -- don't we need to do something more with the decorator to handle arguments? Your tests aren't actually testing "regular" use of the decorator. I.e. in this:
@task(name="foo") def blargh: passThe function
task()needs to return a decorator, which is what then accepts the wrapped function as its sole argument.IIRC there is a common pattern for decorators which detect how they are used and act appropriately, and we could use that in our test suite too for
@server. I forget where this pattern is documented offhand.Feel free to tell me to handle this part myself =) just wanted it documented.
tswicegood
I've updated to allow both
@taskand@task()in a way that works. I'm not happy with it, but it turns the test color green. I'm open to any ideas on how to take a crack at it.
bitprophet
Try
kwargs.pop("task_class", tasks.WrappedCallableTask):D (Or should I be trying to submit my own commits as corrections instead? How does that work with a pull request from you to me?)
tswicegood
Per commit 41eebaa, I think we should remove support for
*argsin favor of simplifying this code. Thoughts?
bitprophet
I am +1 on removing _args -- having *_kwargs only is typically fine for this sort of user facing, readability-focused API.
I still feel we can improve the decorator's implementation overall but I'll look for that prior art I keep referencing, and take a stab myself. Thanks a lot for jumping on this.
tswicegood
Any status update on where this stands? I'd like to see it merged in so I can kill this branch (and quit seeing an open PR in my list of outstanding one's).
bitprophet
Haven't had time to double check / merge it but might over the next few days, depends whether I can finish up some other stuff. Would like to include this in the next mini-release, fits well with adding the aliasing/default task stuff.
tswicegood
Speaking of defaults, now that
@taskcan take kwargs, defaults would look good like that.@task(default=True). I can take a stab at it if ya want.
bitprophet
Yea that's basically my plan, use these args for stuff like default and aliasing. Feel free to try adding those to this implementation.
Re: aliasing: I'm considering having both
@task(alias='foo')and@task(aliases=['foo', 'bar']), so that the "common" case of a single alias doesn't require use of a one-item iterable. But the usual "now the API has 2 funcs instead of one" problem arises. I think it's worth it though.
tswicegood
Agreed -- I think we should have both.
bitprophet
FYI: I'm creating a branch based off of my master, merging what you have already in your branch here, and running with it. Should have something push-worthy by next week.
bitprophet
Note that with this stuff I am experimenting with the "put the real docs in /usage and have a more stripped down/pointer-like docstring" approach.
bitprophet
This ought to be pretty much done now: class+args, defaults, aliases.
A few items remain: top-level defaults (like
make/rake) and gussying up--listoutput to make defaults/aliases more obvious. Should be quick to implement both.Slightly torn on top level defaults (right now I explicitly doc'd that it is not in) as it's sort of backwards incompat:
fabby itself prints out help right now. But it would be opt-in and conceptually consistent with having defaults at every other level, so I think I will do it.
bitprophet
Adding the extra info to
--listoutput is actually not as simple as it felt, and it's not critical, so going to pass on it for now.
bitprophet
Enabling default tasks adds another minor problem: it makes it impossible to print help-by-default when there are no local fabfiles (i.e. typing just
fabin a directory with nofabfile.pyetc.) This is something I remember explicitly fixing in the past (in #75).I think I'll update the "no fabfile!" error to still be a short error message, but with more info about how to access
--helpand that they might have meant to use--fabfile.This is better than just going back to the old "abort instead of help" behavior, but also avoids a problem with printing both full help and the error (namely that the error might be obfuscated by all the help output.)
tswicegood
Ugh -- just realized that this got merged with
*argssupport. I thought we didn't want that? If we remove*argssupport (which only vaguely documented -- almost in an undocumented way wink) we could simplify the task decorator like this: https://gist.github.com/1108573
bitprophet
I don't remember agreeing not to bother with
*args:( I don't really care one way or another (kwargs are way more useful generally) but it is explicitly documented now, albeit almost in passing -- see the task usage docs, 3rd paragraph in that section.I'm -0 on the linked tightening-up -- I honestly expect to get to Fab 2.0 sometime this year and thus all this stuff would be getting cleaned up then anyways. And even though it is barely documented, it is documented and there is a release out with the
*argsbehavior, so it feels kind of mean to yank it back out again just to save a few lines of code.(Also -- I may end up having to nuke this PR/ticket when we migrate -- still working out with the GH people how/whether deletion is possible. Just FYI. Would copy/paste it into the to-be-migrated Redmine ticket 22 if so.)
tswicegood
Copy and paste the full discussion? That's pretty intense. :-)
Re you agreeing to nuke
*args: #22 (comment)
bitprophet
Ah, there it is, I didn't read far enough up :( My bad, then -- I completely forgot about that part of the discussion when I was merging/updating. Is the extra complexity of the decorator implementation causing some sort of additional pain for you now? Otherwise I think it's best to leave it as-is until we smooth all this over in 2.0.
tswicegood
Nope, not causing me pain. I was checking up on Convore last night and remembered this and thought I'd double check. I really thought I had pulled it, but guess I forgot too.
bitprophet
OK, thanks. Oh and re: copy/paste, I was just going to select most of the web page's content and then maybe clean it up a tiny bit. Or use the API ;)
Anyway waiting on an answer from On High about whether it's even possible to truly delete all these and reset the counter or if I am SOL re: importing with the right numbers. In which case I may just do the lock-and-store method, leave Redmine up in a readonly state for a year or so, generate a few hundred dummy closed issues here and start off at the next number from Redmine's newest ticket. Bleargh.
on 2011-08-18 at 07:45pm EDT
- added 13 commits that reference this issue
on Aug 22, 2011
Description
Add ability to specify the task class to use, along with args and kwargs that are passed to the task class.
(Jeff) Also: aliasing and module-default tasks.
N.B. #383 used to be number 22.
Originally submitted by Jeff Forcier (bitprophet) on 2009-07-21 at 03:23pm EDT
Relations
Closed as Done on 2011-08-18 at 07:53pm EDT