Refactor Task and Subsystem dependencies

[jsirois]

I discussed this with @benjyw offline, but the current fork in TaskBase.{global_subsystems,task_subsystems} presents a few challenges including requiring scope derivation by 3rdparties instead of the dependees themselves and not being uniform from TaskBase to Subsystem (Subsystem has a single Subsystem dependecies method). The desired 2 API changes are as follows, and the rest should flow from there:

class ExampleTask(Task):
  @classmethod
  def subsystem_dependencies(cls):
    return (
      Subsystem1,  # A global instance dependency
      Subsystem2.scoped(cls)  # A scoped instance dependency
    )

  def execute(self):
    subsystem1 = Subsystem1.instance()  # The global instance.
    subsystem2 = Subsystem2.scoped(self).instance()  # The scoped instance.

The same mix of global and scoped Subsystems can be depended on by Subsystems themselves using this API with a single dependencies method and a single instance method. The new Subsystem.scoped(optionable) method will return a new Subsystem subclass with the qualified scope baked in such that no calculation of actual scopes in-play will be needed. Instead, the Subsystem.closure can be gathered and the Subsystem (Optionable) types can be directly consulted for their full .options_scope.

[benjyw]

I like this, but I would go even further:

subsystem2 = Subsystem2.scoped(self).instance() is redundant. That information was already provided in subsystem_dependencies().

It would be nice to be able to do:

  def execute(self):
    subsystem1 = self.subsystem(Subsystem1)
    subsystem2 = self.subsystem(Subsystem2)

or

  def execute(self):
    subsystem1 = Subsystem1.instance_for(self)
    subsystem2 = Subsystem2.instance_for(self)

So the author of execute() doesn't even need to think about scoping.

What do you think?

[jsirois]

I'd like that too, but even further - dependency injection. The subsystems should be injected in the Task/Subsystem constructor. You shouldn't even need to ask for the thing 2x. That's a few refactors off though, and folks tend to retch at the mention of DI ao it may be controversial.

Back to your suggestion, it does not allow for mixed scope instances of the same type, but maybe its fine to not allow this. IE: no depending on both the global instance and a scoped on from the same dependee. Putting that aside and considering "normal" use cases, it does seem like Subsystem could hang on to a class-level registry of requestors -> requested Subsystem types.

[benjyw]

T thing I suggested could be a common shortcut, but we'd still allow your
thing.

The latter would be used in the rare case when the author of execute()
truly does need to reason about scoping.

On Tue, Aug 11, 2015 at 10:20 PM, John Sirois notifications@github.com
wrote:

I'd like that too, but even further - dependency injection. The subsystems
should be injected in the Task/Subsystem constructor. You shouldn't even
need to ask for the thing 2x. That's a few refactors off though, and folks
tend to retch at the mention of DI ao it may be controversial.

Back to your suggestion, it does not allow for mixed scope instances of
the same type, but maybe its fine to not allow this. IE: no depending on
both the global instance and a scoped on from the same dependee. Putting
that aside and considering "normal" use cases, it does seem like Subsystem
could hang on to a class-level registry of requestors -> requested
Subsystem types.

—
Reply to this email directly or view it on GitHub
#1957 (comment).

[jsirois]

Good point - I like that.

[jsirois]

As noted in pantsbuild slack #general - This change should support separating the global subsystems from those that have been scoped when examining a dependencies list.

[jsirois]

@benjyw if you want this its yours. I'm still embroiled in #1998 and need to complete a fix for that for tomorrow am-ish.

Just self-assign and label in-progress. Otherwise I'll likely be bumped off hitting this until next weekend.

[benjyw]

It's a bit of a detour, but I do need this for my last #docfixit change, so I'll see if I can do this quick-like.

[benjyw]

Implemented in https://rbcommons.com/s/twitter/r/2653/.