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

Consider making contextualization the default #114

Closed
bitprophet opened this Issue Feb 11, 2014 · 13 comments

Comments

Projects
None yet
8 participants
@bitprophet
Member

bitprophet commented Feb 11, 2014

Occasionally catch myself doing the following:

from invoke import Collection, task

# stuff

@task
def foo(ctx, blah, otherblah):
    ctx.run("whatever")

ns = Collection(foo)

Which leads to a nice 'test' did not receive all required positional arguments! error, because task != ctask. Also I think part of an FAQ mentions this (!)

If it's something I do myself cuz I haven't written a new Invoke task in a month...it's gonna bite users. The un-contextualized use case is and should be the minority; therefore I really should:

  • Update docs to describe a "contextualized by default" setup, including removing mention of @ctask.
    • Maybe be hardcore and just don't even bother with uncontextualized tasks? Eh. Leaving the flag in allows people to rebind the name to an inverted-ctask if they really care about not using the context.
  • Update tests to match
  • Add new tests enforcing a useful "Did you forget a leading context argument?" add-on to the above error message, probably.
  • Implement.
  • Update all my stuff (whee! Good thing most of it is in Invocations...)
  • Add big honkin' backwards incompatibility note to teh changelog (maybe a good new Releases feature?)
@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 22, 2014

Actually thinking this pairs well with #147 because I'm having a heck of a time figuring out how to recast the contexts doc page in light of it not being the main config vector now. Part of this problem is because the entire doc is structured as a "here's why you want to use contexts" and if it's going to need a restructure anyway this might be a good time.

OTOH I've already spent so long on #147...heh.

EDIT: Gonna punt on that for now but this is still something I ought to do soonish :)

@KuangEleven

This comment has been minimized.

Contributor

KuangEleven commented Jun 3, 2016

Working on removing non-contextualized tasks

@thebjorn

This comment has been minimized.

thebjorn commented Jun 4, 2016

Perhaps a stupid question...? but if all tasks need a first argument of a specific type, and it's a source of error that users forget this argument.. Wouldn't it be sensible to define a task as a subclass of Task, since then the first argument would be self which when missing is much more likely to be picked up by linters/IDEs? I know you can define a class as a task already (e.g. https://github.com/datakortet/dk-tasklib/blob/master/dktasklib/lessc.py) and even though it works well it feels a little hacky..

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 4, 2016

@thebjorn Not a stupid question! It's true that making them class definitions would make that line up a bit better. The problem is, this type of tool wants to be as DSL-like & lightweight as possible, so any additional boilerplate drives it farther from that goal.

The use of a context object is already a moderate boilerplate addition (one I judged worthwhile because of the large gains in explicitness, architectural sanity & debug friendliness). Adding class-def lines & extra indent levels strikes me as not quite worth the tradeoff.

Relatedly, though - I realized the other day that our problem is literally the same problem as method self args - including the fact that you get a newbie-unfriendly error if you forget self. Updating the docs to highlight this fact - hoping that users are already used to writing classes and having that mental model - might be useful.

Finally, having methods-as-tasks is something we want to have as an optional feature - see #347. I just don't think that should be the default mode.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 4, 2016

Props to @KuangEleven , this is in now. Bring on the flames! 😒

@classam

This comment has been minimized.

classam commented Jun 21, 2016

Bring on the flames!

... I mean, I understand the dependency injection value of doing this, but UAAUGH. Not only do I have to change every codebase of mine that uses invoke as a simple task runner, but I also have to change an article I wrote about it.

YEARGH. I AM INCONVENIENCED.

... thanks for all of your dedicated work on invoke.

@MinchinWeb

This comment has been minimized.

MinchinWeb commented Jun 30, 2016

The default context variable breaks Invoke on Windows when you go to run shell commands. See #371 for details.

@indera

This comment has been minimized.

indera commented Jun 30, 2016

I respect the @bitprophet's decision to not implement this, but I expect that this issue will be fixed in the future...

Maybe by a dedicated decorator -- to avoid passing the ctx, or by sub-classing as suggested by @thebjorn

@task
def normal_task_not_requiring_run():
pass

@RTask
def task_invoking_shell_commands():
run()

Also it would be interesting to know how many users decided to try invoke just because it allows to invoke shell commands :)

@code-tree

This comment has been minimized.

code-tree commented Jul 1, 2016

I assume the reason for this change is a good one, but practically speaking, what is the best practice for converting this old code to the new standard?

@task
def do_in_source_dir(cmd):
    """ Do command within source dir """
    with change_to_source_dir():
        run(cmd)

@task
def gitk():
    """ Open gitk within source dir """
    do_in_source_dir('gitk --all')

I have a great number of calls to tasks within other tasks, as they need to pass on the arg they receive to the other task (not merely run before/after. Any ideas on how to refactor this?

thanks!

@indera

This comment has been minimized.

indera commented Jul 1, 2016

Tedious to refactor but passing "ctx" around would do it:

from invoke import task

@task
def do_in_source_dir(ctx, cmd):
    """ Do command within source dir """
    ctx.run(cmd)

@task
def gitk(ctx):
    """ Open gitk within source dir """
    do_in_source_dir(ctx, 'ls -al')
@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 3, 2016

@code-tree The quick way for now is to pass the initial context around as noted by @indera. Once #170 is solved, that'll be the "right" way, and ideally it'll be something like ctx.call('other-task') (replacing literal other_task() calls as you do now).

@pekkaklarck

This comment has been minimized.

pekkaklarck commented May 22, 2017

I'd be very happy to have a separate task decorator that could be used without a context. I'm building a library of generic tasks and utility functions for my projects and context objects make API design somewhat annoying:

  • Tasks need context as the first argument although most of them don't actually use.
  • Very few utilities need context at all. Passing it only to them makes APIs inconsistent but unnecessary passing them to all makes APIs ugly.

I'd also like to understand in which cases the context actually gives benefits for users. I've only used ctx.run but don't see how it's better than the old invoke.run. I browsed through the docs but couldn't find examples other than that ctx.run.

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 23, 2017

Thanks for the feedback, @pekkaklarck! Appreciate it. I continue to be of two minds about adding it back (it actually used to be the case that it was optional) but I suspect if we bring it back as the opt-in, non-default behavior, that might solve everyone's problems:

  • The common case in the docs, etc, continues to be the 'contextualized' one
  • New users who don't have a strong preference won't shoot themselves in the foot as easily
  • Users who do have a strong preference will be opting-in (ideally after reading a brief doc noting that they'll lose most of the feature set like access to the loaded configuration) & can import a 'contextless' task decorator
  • As a side note, we're currently looking at using multiple task decorators in Fabric 2 (rather, having Fabric add its own task decorator so users with mixed local & remote tasks can clearly separate them) so if we do that, it becomes an established pattern to "pick your task type".

So I'll think about that some more prior to 1.0.0.


To answer your question as to why contexts are "good":

From a pure-Invoke perspective they function as a way to access the configuration system, which otherwise would require some sort of global variable.

The other big reason is for client libraries such as Fabric 2 where Context is subclassed to provide an even more 'active' "task execution context", like server connections, where it's not only got a run method but other methods like open, put, get etc. Similarly such libraries often want to parameterize tasks by calling them N times, each with a distinct context object (e.g. iterating over a series of remote hosts.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment