-
Notifications
You must be signed in to change notification settings - Fork 40
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
Allow running the specs of external huginn agent gems #2
Conversation
05d86fc
to
f22b823
Compare
I'll try this out myself shortly! |
end | ||
end | ||
|
||
def register(*paths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than providing paths to look for agents, could we structure this such that Agents "register" themselves as they're loaded? Something like:
class MyAgent < HuginnAgent
add_self_to_registry
...
end
class HuginnAgent
cattr_accessor :agent_types
agent_types = []
def self.add_self_to_registry
agent_types |= [self]
end
...
end
Or, per huginn/huginn#1526 just call HuginnAgent.descendants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Agents need to inherit from Agent
(the AR base model), so to allow this kind of registration HuginnAgent
would need to be a subclass of Agent
which is not defined yet when the gem is loaded into Huginn. Maybe I am missing something but due to the load order I think that would require us to monkey patch (predefine) the Agent class before its defined in Huginn.
What I like about the approach that @cantino took is that the Agent code does not need any changes to be move into a gem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that it's nice how little code change was needed. Agents definitely should stay subclasses of Agent for now, I think. All of our AR code assumes this. It'd be okay to add an (optional?) registration call made at the class level when an Agent is defined, though. Or just a class method that sets a class variable indicating the group or something @TildeWill?
Can we get this merged? I know this PR introduces a lot of new code, but most of it is for the agent gem generator. The only code path that is run by Huginn is this method which doesn't do anything when the ADDITIONAL_GEMS .env variable is not configured, so it will definitely not break existing installations. |
I'm traveling, but will try to look at it tonight. |
Yes, looks good to me @dsander! I haven't manually tested it, but it all makes sense. Go ahead and merge. |
f22b823
to
2354549
Compare
2354549
to
081e97f
Compare
@cantino Thanks! Now we only need to push the new version to rubygems to merge the Huginn side of this :) |
@dsander, I just pushed v0.4.0 to RubyGems. I tried to add you on RubyGems as an owner and it said that it couldn't find your |
@cantino Awesome! It should be git (at) dsander.de |
Added you! |
Only created to reference it from the Huginn PR, I think we should keep the discussion there and continue here after we agree to pursue this approach.