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

Support for XMPP rosters on connect #65

Closed
wants to merge 13 commits into from

Conversation

farski
Copy link

@farski farski commented Mar 16, 2014

I'm trying to eliminate the need for a running a fork of hubot-xmpp just to enable (#41) roster support. The change is pretty minor. The goal here is still to allow for a script like PRX/party-line-hubot@b838526 to work as a relay/broadcast system over XMPP.

The extra conditional in readIq I think it pretty straight forward. I have my doubts about how I exposed @robot.xmppRoster and @robot.xmppClient. The client may have already been accessible from the robot and I just wasn't sure how, and it may make more sense for the roster to be a property of the client, not the robot. Looking for feedback on either of those points.

If there's a way to accomplish this by extending hubot-xmpp in another library, rather than making this change here directly, I can go that route as well. I just wasn't sure how to intercept the readIq block in a good way to do that.

I will add some tests before this gets merged.

# Expose the XMPP client through the robot to allow third-party scripts
# to send messages directly
@robot.xmppClient = @client
@robot.xmppRoster = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the robot had a reference to the adapter it was using. I could be wrong though.

Copy link
Author

Choose a reason for hiding this comment

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

Apparently there is a robot.adapter. I will make sure it works the same as what I'm doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

You would need robot.adapter.client.

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed that robot.adapter.client works

@markstory
Copy link
Contributor

Your most recent commits have failing tests.

@farski
Copy link
Author

farski commented Mar 17, 2014

There were some issues with my original PR, and the script I'm using this with needed to get caught up to a newer version of hubot, so I'll have to do some more work on this tonight.

@markstory
Copy link
Contributor

If this had some tests, I would be happy to merge it.

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

Successfully merging this pull request may close these issues.

2 participants