Next attempt to pull Agents into gems #1366

dsander · 2016-03-23T12:44:15Z

The work is based on #920 and cantino/huginn_agent.

While I generally liked the idea of #1012 to add a "base agent" class which decouples the agent gems from active record it also added problems, for the specs to pass every method call that needs active record has to be mocked, stubbed or abstracted in the base agent class.

This attempt follows #920 by only moving the agent classes and specs into a gem and load them on startup. To ensure the agent gem works with Huginn I added a (at the moment messy) rake task which clones Huginn, injects the agent gem and runs the specs of the gem.

Running the tests on travis works as well: https://travis-ci.org/kreuzwerker/DKT.huginn_nlp_agents

The source of the gem can be viewed here, the only change needed was adding require 'huginn_agent/spec_helper' (which requires all files in spec/support/) to the agent specs.

The pull request to huginn_agent can be viewed here: huginn/huginn_agent#2

If we agree to follow this approach I am planning to make rake task universal and output more useful by shelling out using open3. Additionally adding a generator which creates the needed file structure for a new agent by running huginn_agent new awesome_agent would be cool.

Thanks to kreuzwerker.de who are paying me to work on this in the context of the research project 'Digitale Kuratierungstechnologien' of which they are a part of.

davidwabnitz · 2016-03-23T17:36:16Z

linking to discussion in #60

cantino · 2016-03-24T02:43:10Z

Very cool! I'll dig into this soon.

dsander · 2016-03-30T12:26:51Z

I added a generator to the huginn_agent gem. huginn_agent new test_agent will create a new agent gem skeleton and attempt to add the needed .env configuration (assuming a .env was found in either ./huginn/.env, .env or ~/huginn/.env).
Running bundle and bundle exec rake inside the new agent directory should run the specs without issues.

darrencauthon · 2016-03-30T20:01:12Z

While I generally liked the idea of #1012 to add a "base agent" class which decouples the agent gems from active record it also added problems, for the specs to pass every method call that needs active record has to be mocked, stubbed or abstracted in the base agent class.

Ah, just for the record... the problems you're associating with that approach are a byproduct of unit testing code, not of the approach to abstracting out the code into a base class. Even when I've written agents or contributed code to projects, I've had to use that same mock/stub/abstract approach to building my test scenarios. Like this:

https://github.com/cantino/huginn/pull/1209/files#diff-026ea14cf5cd2e2f57acd81960c31114R30

It's just my personal position on testing, and a conclusion I've come to after writing thousands upon thousands of tests. I know it's not shared by most Rails developers (Rails testing starts and ends with integration tests, even to the point that its concept of "unit tests" still hit the database), which is fine. We all pay the consequences of our own decisions, and I'm learning not to get worked up over these differences of view. 😄

I think that mocking and stubbing is great, as it frees me from the details that I don't care about -- like active record's serialization of hashes and the like. I'm using the framework, so I'm going to assume they work and instead focus on my code. I gain flexibility and simplicity in my tests, at the cost of the coverage that integration testing gives me.

Anyway, I just wanted to explain that bit. I really enjoyed the work I put into those pull requests, as they gave me a new look into abstracting by "projecting" an one thing (like my Rails-free BaseAgent) into an entirely different thing (like the Rails agent). Ruby is a very powerful and fun language! ⚡ ⚡ ⚡

I like your approach, it's a very neat idea. Good luck! 😄

cantino · 2016-03-31T02:40:21Z

I'm on vacation next week and looking forward to digging into this :)

cantino · 2016-04-13T20:30:57Z

This took surprisingly little code!

dsander · 2016-04-13T21:37:07Z

@cantino I was surprised as well, do you think the approach is worth pursuing?

cantino · 2016-04-14T03:49:19Z

It seems like it. The only real downside is that we lose the ability to update all Agents with a single commit, but since people may have private Agents, we don't really have that now anyway.

dsander · 2016-04-14T13:09:45Z

I think we should keep our current agents in this repository and maybe move some with big dependencies into a agents directory like you did with the RssAgent in your PR.

What I am still uncertain about is how we will let the user add additional agent gems to the Gemfile. Something like HUGINN_AGENT_n=huginn_nlp_agents;~> 0.2.1;git://... could work.

cantino · 2016-04-16T03:53:16Z

You mean adding Agents in the Docker context?

dsander · 2016-04-16T07:48:57Z

Yes for docker users the gems have to be configurable via the environment. For the other installation methods it would still be helpful otherwise you would need to maintain a small fork (even if the changes would just be in Gemfile*)

cantino · 2016-04-16T15:45:57Z

Hmm, good question.

I might suggest parens for the version strings.

HUGINN_ADDITIONAL_GEMS=huginn_nlp_agents(~> 0.2.1),huginn_slack,huginn_uber(> 1.0)

dsander · 2016-04-16T16:22:47Z

That should work, maybe optional square brackets for a path or git url?

HUGINN_ADDITIONAL_GEMS=huginn_nlp_agents(~> 0.2.1)<git://github.com/...>,huginn_slack,huginn_uber(> 1.0)

cantino · 2016-04-16T22:26:38Z

Torn between that and something like

HUGINN_ADDITIONAL_GEMS=huginn_nlp_agents(v: ~> 0.2.1, url: git://github.com/...),huginn_slack,huginn_uber(v: > 1.0)

or even

HUGINN_ADDITIONAL_GEMS=huginn_nlp_agents(~> 0.2.1, git://github.com/...),huginn_slack(git@...),huginn_uber(> 1.0)

because we can probably detect a GIT / URL vs a version.

dsander · 2016-04-19T09:14:35Z

I went with a more verbose version which allows possible arguments of the gem command. Did I forget cover a use case?

cantino · 2016-04-20T02:50:56Z

lib/gemfile_helper.rb

+    GEM_SEPARATOR = '\s*(?:,|\z)'.freeze
+    GEM_REGULAR_EXPRESSION = /(#{GEM_NAME})(?:\(#{GEM_OPTIONS}\)){0,1}#{GEM_SEPARATOR}/
+
+    def parse_agent_gems(string)


Maybe parse_each_agent_gem(string)?

dsander · 2016-04-21T13:36:03Z

I would consider this feature complete now. Most of the changes are in huginn/huginn_agent#2. Maybe we should move the discussion over there and continue here after huginn_agent PR is merged?

darrencauthon · 2016-04-21T19:36:36Z

Gemfile

@@ -192,3 +193,7 @@ end
 if_true(ENV['DATABASE_ADAPTER'].strip == 'mysql2') do
  gem 'mysql2', '~> 0.3.20'
 end
+
+GemfileHelper.parse_each_agent_gem(ENV['ADDITIOANL_GEMS']) do |args|


@dsander Just in case... should this be ADDITIONAL_GEMS?

Yes 😊, thanks!

dsander · 2016-06-20T09:20:47Z

The specs are only failing because I bumped the huginn_agent version to 0.3.0 which is not release to rubygems yet. https://travis-ci.org/cantino/huginn/builds/138849715 is the last build.

cantino · 2016-06-20T19:05:08Z

lib/gemfile_helper.rb

@@ -19,8 +19,33 @@ def load_dotenv
                              )
    end

+    GEM_NAME = '[A-Za-z0-9\.\-\_]+'.freeze
+    GEM_OPTIONS = '(.+?)\s*(?:,\s*(.+?)){0,1}'.freeze
+    GEM_SEPARATOR = '\s*(?:,|\z)'.freeze


Should it be \s*(?:,\s*|\z) in case there are spaces after the comma?

It's not needed, spaces are not valid in the GEM_NAME expression they will be .scan will skip over them and start the next match when any character that is allowed in GEM_NAME is found. In the specs gem are separated with spaces.

dsander · 2016-06-21T08:47:14Z

Merged! Thanks for the feedback and reviews!

dsander force-pushed the feature/agents_in_gems branch 2 times, most recently from 46ec385 to ce9ad90 Compare April 11, 2016 07:54

dsander force-pushed the feature/agents_in_gems branch from ce9ad90 to 1d19353 Compare April 19, 2016 09:10

dsander force-pushed the feature/agents_in_gems branch from 135231c to 0485164 Compare April 19, 2016 13:33

cantino reviewed Apr 20, 2016
View reviewed changes

dsander changed the title ~~WIP: Next attempt to pull Agents into gems~~ Next attempt to pull Agents into gems Apr 21, 2016

darrencauthon reviewed Apr 21, 2016
View reviewed changes

dsander force-pushed the feature/agents_in_gems branch 2 times, most recently from 2d392c6 to 708106a Compare April 27, 2016 09:02

dsander force-pushed the feature/agents_in_gems branch from 708106a to 65ba4b4 Compare May 13, 2016 09:35

dsander mentioned this pull request May 30, 2016

Decouple agent types from file structure #1526

Closed

cantino and others added 2 commits June 20, 2016 10:32

initial effort on pulling Agents into gems

0bd46b5

Move RSSAgent back to its original path

9c8ec30

dsander added 3 commits June 20, 2016 10:32

update huginn_agent

f25b308

change huginn_agent git repository

603f3e7

Allow specifing Agent gem in the Gemfile

811f9c2

dsander force-pushed the feature/agents_in_gems branch 3 times, most recently from 0d24f5d to d830459 Compare June 20, 2016 09:20

cantino reviewed Jun 20, 2016
View reviewed changes

Bump huginn_agent version

222f3a4

dsander force-pushed the feature/agents_in_gems branch from d830459 to 222f3a4 Compare June 21, 2016 08:23

dsander merged commit bba0e2e into huginn:master Jun 21, 2016

dsander deleted the feature/agents_in_gems branch June 21, 2016 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Next attempt to pull Agents into gems #1366

Next attempt to pull Agents into gems #1366

dsander commented Mar 23, 2016

davidwabnitz commented Mar 23, 2016

cantino commented Mar 24, 2016

dsander commented Mar 30, 2016

darrencauthon commented Mar 30, 2016

cantino commented Mar 31, 2016

cantino commented Apr 13, 2016

dsander commented Apr 13, 2016

cantino commented Apr 14, 2016

dsander commented Apr 14, 2016

cantino commented Apr 16, 2016

dsander commented Apr 16, 2016

cantino commented Apr 16, 2016

dsander commented Apr 16, 2016

cantino commented Apr 16, 2016

dsander commented Apr 19, 2016

cantino Apr 20, 2016

dsander commented Apr 21, 2016

darrencauthon Apr 21, 2016

dsander Apr 22, 2016

dsander commented Jun 20, 2016

cantino Jun 20, 2016

dsander Jun 20, 2016

cantino Jun 20, 2016

dsander commented Jun 21, 2016

Next attempt to pull Agents into gems #1366

Next attempt to pull Agents into gems #1366

Conversation

dsander commented Mar 23, 2016

davidwabnitz commented Mar 23, 2016

cantino commented Mar 24, 2016

dsander commented Mar 30, 2016

darrencauthon commented Mar 30, 2016

cantino commented Mar 31, 2016

cantino commented Apr 13, 2016

dsander commented Apr 13, 2016

cantino commented Apr 14, 2016

dsander commented Apr 14, 2016

cantino commented Apr 16, 2016

dsander commented Apr 16, 2016

cantino commented Apr 16, 2016

dsander commented Apr 16, 2016

cantino commented Apr 16, 2016

dsander commented Apr 19, 2016

cantino Apr 20, 2016

Choose a reason for hiding this comment

dsander commented Apr 21, 2016

darrencauthon Apr 21, 2016

Choose a reason for hiding this comment

dsander Apr 22, 2016

Choose a reason for hiding this comment

dsander commented Jun 20, 2016

cantino Jun 20, 2016

Choose a reason for hiding this comment

dsander Jun 20, 2016

Choose a reason for hiding this comment

cantino Jun 20, 2016

Choose a reason for hiding this comment

dsander commented Jun 21, 2016