search

slack-rs / slack-rs-api

Rust interface for the Slack Web API
Apache License 2.0
110 stars 67 forks source link

Remove ListResponseItemMessage::message #71

Open bobrippling opened 5 years ago

bobrippling commented 5 years ago

The reaction_added and reaction_removed events don't have message as a field, which was causing the deserialization to fail, preventing upstream libraries from handling reactions.

fixes slack-rs/slack-rs#111

dten commented 5 years ago

The code you've edited is generated from the schema. It's really round about but the proper change is to edit the schema, updated the the submodule in this repo, generate the code and pr

bobrippling commented 5 years ago

Ah okay - I've opened a PR (slack-rs/slack-api-schemas#15) in the schema repo, although it looks like one of my fixes duplicates an existing PR (slack-rs/slack-api-schemas#8).

However, even with this schema update, the line pub message: ::Message, remains in src/mods/reactions.rs, preventing us from handling reaction messages. :(

dten commented 5 years ago

I'm trying to confirm this, which method are you trying to call that is failing?

dten commented 5 years ago

i think the issue is related to this comment in the docs which imply the event version of the item does not contain as much info. So it looks like message is optional

Embedded item objects
Embedded item nodes are more lightweight than the structures you'll find in reactions.list.

looking further it's the reuse of the list items as the rtm items when they're really different types

bobrippling commented 5 years ago

Yeah, I think making message optional is the right fix.

To reproduce the issue, I wrote an example in my fork of slack-rs, on the reaction-example branch. That branch uses this PR for its slack-rs-api dependency, and the example logs all reactions it receives.

If you check this out and run the reaction example, note that slack reactions are logged. Then if you revert the changes to Cargo.toml so it uses the upstream slack-rs-api code, you should see that reactions are no longer logged, as the on_event() method is never fired for them.

Let me know if you have any trouble with this.

dten commented 5 years ago

Ah yes I did get a repro in the end. There are some other events also such as pin added that suffer from the same problem. Just trying to make sure they're all caught and will get it fixed. Might expose an easy way to get the failures so I can have my app reporting all failures somewhere to be dealt with later