Skip to content
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

Twitter retweet agent #1181

Merged
merged 7 commits into from
Apr 26, 2016
Merged

Twitter retweet agent #1181

merged 7 commits into from
Apr 26, 2016

Conversation

id4ho
Copy link
Contributor

@id4ho id4ho commented Dec 13, 2015

@cantino Thought I'd take a crack at this one.

This agent is meant to only consume events (tweet events at that). Whats the convention of generating success/failure events? I saw they were used in the publish agent. Have a couple questions that I'll add as comments in the code.

Still learning my way around and happy to make any necessary changes, or add obvious functionality that I may have overlooked. I did run this successfully locally.

@@ -36,7 +36,7 @@ def twitter_oauth_token_secret
end

def twitter
Twitter::REST::Client.new do |config|
@twitter ||= Twitter::REST::Client.new do |config|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to mock a method on twitter in my spec and generating a new one every time was not allowing that. I could use any_instance_of(Twitter::REST::Client) but I didn't see a reason not to cache this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine-- we'd be making a new one for any instance of a new Agent anyway.

@cantino
Copy link
Member

cantino commented Dec 17, 2015

Thanks @jackvnimble! :)

'success' => false,
'error' => e.message,
'tweets' => Hash[tweets_to_retweet.map { |t| [t.id, t.text] }],
'agent_id' => incoming_events.first.agent_id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do agent_ids instead, or make an event per event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and event_ids. This should be fixed now.

@cantino
Copy link
Member

cantino commented Dec 20, 2015

Let me know when this is ready. No rush!

@knu
Copy link
Member

knu commented Dec 21, 2015

Just an idea, but what about merging TwitterPublishAgent and this into TwitterActionAgent? There are some other possible actions like follow and like, and adding one agent for each action may look messy.

@id4ho
Copy link
Contributor Author

id4ho commented Dec 22, 2015

@knu That's a good suggestion and I actually mentioned this to @cantino and we decided that separate agents would be ok (at least for now).

I can see what you mean about having one action for each being messy, but I think trying to have one agent responsible for publishing tweets, following, favoriting and retweeting may be a bit hard to use for someone just trying to use Huginn (and not understand the code underneath).

To me, TwitterActionAgent seems like it would be a good logical place to include both retweets and likes as both require tweet ids. TwitterPublishAgent on the other hand just needs a message and can consume many different types of events other than tweets. Following also seems a bit different but since tweets have user_ids maybe it'd make sense in a TwitterActionAgent too. What do you think?

@knu
Copy link
Member

knu commented Dec 22, 2015

OK, it makes much sense in that it would be user-friendlier to have separate agents. I agree actions like follow/like/mute/unfollow should be separated from publishing actions.

I thought about the idea of merging with TwitterPublishAgent because I had the "retweet with comment" feature in mind, but it seems to be just a tweet with a reference to another tweet, so TwitterPublishAgent will be able to do that without modification.

@cantino
Copy link
Member

cantino commented Dec 24, 2015

Let us know when this is ready. As always, no rush-- it's just hard to tell when people consider a PR to be finished. Happy holidays!

@id4ho
Copy link
Contributor Author

id4ho commented Jan 1, 2016

@cantino Just speaking to retweet, I think this is good to go. However, I'd like to incorporate like and follow as suggested by @knu. If we change the name of the agent to TwitterActionAgent in a later PR, I assume this will create some problems for anyone using it (maybe not if we include a migration/backfiller)?

If that assumption is correct, maybe it's best to wait until I can add the other actions, or go ahead and change the name and then add the actions in another PR. What do you think?

Happy Holidays!!

@cantino
Copy link
Member

cantino commented Jan 3, 2016

Agreed, let's rename before we merge it in.

@bricemaurin
Copy link

HI @cantino / @jackvnimble: any idea when this one will be merged with master ?
thx !

@cantino
Copy link
Member

cantino commented Apr 19, 2016

@jackvnimble, other than our conversation about separating Agents, was this ready?

@id4ho
Copy link
Contributor Author

id4ho commented Apr 21, 2016

@cantino and all, sorry for the delay on this. I actually have a branch that does the rename and adds a favoriting option. Just need to touch it up and push it up. Should be able to do that tomorrow morning.

On Apr 18, 2016, at 8:26 PM, Andrew Cantino notifications@github.com wrote:

@jackvnimble, other than our conversation about separating Agents, was this ready?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@id4ho
Copy link
Contributor Author

id4ho commented Apr 23, 2016

@cantino this should be ready (pending a code review and passing build).

I've added the ability to favorite tweets and renamed to TwitterActionAgent.

@id4ho id4ho force-pushed the twitter_retweet_agent branch 2 times, most recently from 63885cc to 756b1d4 Compare April 23, 2016 21:06
end
end

def build_agent(options)
Copy link
Member

@cantino cantino Apr 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be scoped inside of the top describe so as to not be global.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, good catch! Fixed.

@cantino
Copy link
Member

cantino commented Apr 24, 2016

Looks great! Just one minor comment. Thanks @jackvnimble :)

This restructures the Agent slightly to allow for retweeting and
favoriting. It is possible to do both at the same time.

- Renames the Agent from TwitterRetweetAgent
to TwitterActionAgent.
- Specs refactored
@cantino cantino merged commit 4147435 into huginn:master Apr 26, 2016
@cantino
Copy link
Member

cantino commented Apr 26, 2016

Great addition, thanks @jackvnimble!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants