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

Fix for decoding reaction_added #83

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bwasty
Copy link

@bwasty bwasty commented May 12, 2017

Related PR with fix: slack-rs/slack-rs-api#43.
Added test case based on a real message that could not be parsed.

@bwasty
Copy link
Author

bwasty commented May 12, 2017

Found another issue (see c4e2e9d) - so now this contains also a fix, not just a test.

@bwasty bwasty changed the title Add test for decoding reaction_added Fix for decoding reaction_added May 12, 2017
@BenTheElder
Copy link
Member

Just noting since I forgot to previously: I am following this and I'll take another look once the upstream changes are sorted and we get travis passing. Thanks for the contributions!

@@ -213,7 +213,7 @@ pub enum Event {
user: String,
reaction: String,
item: Box<reactions::ListResponseItem>,
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the PR on slack-rs-api, this object should be replaced with the lighter-weight object as specified in the Slack event docs. No upstream change necessary! :)

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I'll do the changes later this week.

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.

3 participants