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

Plugins updated + Heroku config #1

Merged
merged 1 commit into from
Jun 10, 2014
Merged

Conversation

fsalum
Copy link
Contributor

@fsalum fsalum commented Jun 8, 2014

Moved out from the msg.get in plugins so they would be compatible with the
daemon version I'm porting it to use with Amazon SQS integration.
Added info to use Heroku to host app for free.

My daemon version is here: https://github.com/fsalum/slackbot-python

@llimllib
Copy link
Owner

llimllib commented Jun 9, 2014

Love the Heroku config, but I don't get why you need to pass the message text, instead of the request form, into the plugins.

My problem with that is that it prevents the plugin writer from accessing any of the other fields in the request; what if they want to whitelist/blacklist users, for example?

@fsalum
Copy link
Contributor Author

fsalum commented Jun 9, 2014

I was afraid of that.

I was trying to make it compatible with the 'daemon' version since I don't
have a request.form.get on it to use for the plugins, see
https://github.com/fsalum/slackbot-python/blob/master/bot.py#L104

my 'body' variable is similar to your 'request.form' variable, but mine is
in json format.

Do you think in any way to make the body and request.form similar so we can
treat them with the same approach in the plugins?

On Sun, Jun 8, 2014 at 8:12 PM, Bill Mill [email protected] wrote:

Love the Heroku config, but I don't get why you need to pass the message
text, instead of the request form, into the plugins.

My problem with that is that it prevents the plugin writer from accessing
any of the other fields in the request; what if they want to
whitelist/blacklist users, for example?


Reply to this email directly or view it on GitHub
#1 (comment).

@llimllib
Copy link
Owner

llimllib commented Jun 9, 2014

It looks to me like we each have a dictionary-like object with a "text" attribute containing the text of the message. Why wouldn't we be OK just passing that in as the msg parameter?

@fsalum
Copy link
Contributor Author

fsalum commented Jun 9, 2014

Yes we should be ok. Let me test that and update the PR.

@fsalum
Copy link
Contributor Author

fsalum commented Jun 10, 2014

PR updated to use the dictionary-like in the plugins instead of request.form.get so it works the same as I will do for the slackbot daemon

@llimllib
Copy link
Owner

Sweet! Now you actually don't need to change the plugins at all; .get exists on dictionaries, and the .get("text", "") protects us from KeyNotFound errors in case Slack sends us a malformed response.

@fsalum
Copy link
Contributor Author

fsalum commented Jun 10, 2014

Learning something everyday :)

I will revert that part and update the PR.

Sent from my iPhone

On Jun 10, 2014, at 7:40 AM, Bill Mill [email protected] wrote:

Sweet! Now you actually don't need to change the plugins at all; .get exists on dictionaries, and the .get("text", "") protects us from KeyNotFound errors in case Slack sends us a malformed response.


Reply to this email directly or view it on GitHub.

@fsalum
Copy link
Contributor Author

fsalum commented Jun 10, 2014

Ok let only Heroku setup now ;)

llimllib pushed a commit that referenced this pull request Jun 10, 2014
Add Heroku config and doc

* Many thanks @fsalum!
@llimllib llimllib merged commit df0631a into llimllib:master Jun 10, 2014
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.

2 participants