Decouple agent types from file structure

[TildeWill]

This is a step in a direction where agents can register what agent group(s) (channels) they're a part of. That will be a later pull-request to clear the way for #1281 where there are several Lifx agents.

To do that, it seemed to make sense to pull out the AgentBuilder into a separate class, and to make the types list less connected to the file structure.

With these changes I think it would now be possible/simpler for someone to pull agents out into a gem.

Drawbacks to these changes: The AssignableTypes concern is now very coupled to Agent as a base class. AgentBuilder also makes assumptions about the presence of an Agent class.

My personal preference would be to turn eager-loading on for dev and test and remove the initializer, but that seemed like a bigger change than was needed for this pull-request.

Related issues #293

[cantino]

I think this is a good idea. We just need to make sure it can work with https://github.com/cantino/huginn_agent/pull/2/files#diff-09a37a6363ac57a7610bef950ca81fc5R31

[TildeWill]

The split is already underway, awesome! Looks like you're making a new HuginnAgent rather than just Agent, so some tweaking would have to happen. As long as agents all inherit from one parent then using descendants seems like a reasonable approach.

[dsander]

I don't think this will be compatible with #1366 and huginn/huginn_agent#2, it would require to completely rethink and rewrite the Agent registration. (https://github.com/cantino/huginn_agent/pull/2/files/f22b823ef0b34c448accb01137ae917c28551746#r65042818)

[dsander]

@TildeWill I think I don't fully understand the bigger picture, do you think about Agent groups/channels in the UI or in the code?

My personal preference would be to turn eager-loading on for dev and test and remove the initializer, but that seemed like a bigger change than was needed for this pull-request.

That would be nice indeed, can you check if the code reloading in dev still works for you?

[TildeWill]

@dsander In the UI I want to be able to present "Groups of agents" rather than a long list. 66+ agents in a single list doesn't offer much discoverability. For the LIFX agents specifically, the feeling was that having one agent per action overwhelmed the list with a block of LIFX agents.

What I was aiming toward was a design where Agents register themselves including their constantized type, human readable name, and group(s). That way if agents live in the main code base, come from a gem, or get loaded some other way, the code doesn't have to know about any of the paths. Agents get loaded and the list gets longer.

[cantino]

I assume they'd still be subclasses of Agent, though?

[dsander]

@TildeWill That sounds like a cool idea are you planning something like the Zapier multi-step interface?

That way if agents live in the main code base, come from a gem, or get loaded some other way, the code doesn't have to know about any of the paths. Agents get loaded and the list gets longer.

I think the loading of the Agent source files is kind of a chicken-and-egg problem, something needs to needs to know where the files are located, previously it was handled by the rails auto loader (which also handled the code reloading in dev) and now you are requiring them explicitly by path.
Is your plan to move the agents out of the app/models/agents directory? I don't see how the current behavior prevents us from structuring the agents into groups, using an additional namespace like Agents::Lifx::EffectAgent (which would then be in app/models/agents/lifx/effect_agent.rb) could be an option.

[TildeWill]

Here's another set of changes still along the lines of agents "registering" themselves in a central place. Eager loading is turned on for all environments which means Rails takes care of loading up all files, triggering the registration. I think what this means is that agents could now live in any directory or even in external gems.

[dsander]

Could we get rid of the AgentRegistry.register_agent(self) calls by utilizing self.inherited(class) in the Agent model?

[TildeWill]

@dsander If I understand you correctly, the goal is that we'd only have AgentRegistry.register_agent(self.inherited(class)) in the Agent class, and not in every subclass of Agent?

I was aiming for each class being explicit about it's inclusion in the registered types. For instance, there could be an "abstract" TwitterAgent class that inherits from Agent that isn't meant to be instantiated, but would have subclasses like TwitterRetweetAgent which inherits from TwitterAgent that is meant to be instantiated.

[dsander]

Something like that, yes. I feel its kind of verbose to have the same line in every agent.

class Agent
  def self.inherited(child)
    AgentRegistry.register_agent(child)
  end
end

I was aiming for each class being explicit about it's inclusion in the registered types. For instance, there could be an "abstract" TwitterAgent class that inherits from Agent that isn't meant to be instantiated, but would have subclasses like TwitterRetweetAgent which inherits from TwitterAgent that is meant to be instantiated.

Do you have a use case in mind that can not be solved with concerns?