Breaking Change Proposal: Use newtype enum pattern for event

[TedDriggs]

Before submitting a very backwards-incompatible pull request, I'd be interested to get people's opinions on moving from enum struct variants to enum "newtype" variants for the Event enum, similar to how the slack_api::Message enum is implemented today.

#[derive(Debug, Clone, Deserialize)]
pub struct Ts(String);

pub trait ChannelEvent {
    fn channel(&self) -> &str;
}

#[derive(Debug, Clone, Deserialize)]
#[serde(tag = "type")]
pub enum Event {
    #[serde(rename = "hello")]
    Hello,
    
    #[serde(rename = "message")]
    Message(Message),
    
    #[serde(rename = "channel_marked")]
    ChannelMarked(ChannelMarked),
}

#[derive(Debug, Clone, Deserialize)]
pub struct ChannelMarked {
    channel: String,
    ts: Ts,
}

impl ChannelEvent for ChannelMarked {
    fn channel(&self) -> &str {
        self.channel
    }
}

Having types per message type would make writing functions to operate on specific message types easier, and still supports matching directly in the main match, albeit at the expense of duplication of the type name.

The downside, of course, is that this is completely backwards incompatible with the existing enum. There'd need to be a way to keep compatibility for the three dependent crates, ideally without permanently abandoning the Event name.

[compressed]

I was actually thinking a similar thing when I was doing some work getting rid of the openssl dependency (https://github.com/compressed/slack-rs/blob/tungstenite/src/events.rs) and moving the code to serde.

I guess the downside is you'd add a bunch of extra structs...

What's the need for the ChannelEvent trait? There's probably no harm in just making all the struct variants be public with pub fields. I think it would be hard for a trait to abstract all response types; e.g. some responses don't even include channel.

I'm also not sure it makes sense to introduce the Ts type if it's just wrapping String. Ideally, we'd probably want to use chrono or something to get an actual useful timestamp there.

[TedDriggs]

What's the need for the ChannelEvent trait? There's probably no harm in just making all the struct variants be public with pub fields. I think it would be hard for a trait to abstract all response types; e.g. some responses don't even include channel.

I was thinking about implementing something like a notification client, and it would be useful there to have a function like:

fn do_things_with_channels<E: ChannelEvent>(evt: &E) {
    // do stuff
}

You're right that the example has written has a problem: If the channel field is made public then the trait method shouldn't be called channel.

I'm also not sure it makes sense to introduce the Ts type if it's just wrapping String. Ideally, we'd probably want to use chrono or something to get an actual useful timestamp there.

The Ts type was a footnote from my looking at #20 - I'm still trying to find documentation about how exactly to interpret the part after the decimal.

[TedDriggs]

@compressed I'll keep an eye on this; happy to help out once the OpenSSL dependency is gone. I can't get it to build on my Mac because of that, and I've run out of time to debug that issue further.

[TedDriggs]

I guess the downside is you'd add a bunch of extra structs...

I've just published a crate to help with this situation: from_variants. It takes care of generating conversion code from each of the structs for the master enum. It's still a lot of structs, but this may help remove some of the boilerplate.

[Mange]

I like the idea, especially with traits for the common fields. On naming conflicts like channel you could go for channel_id() -> &str (or channel_id() -> &ChannelID).

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct ChannelID(String);

trait ChannelMessage {
  fn channel_id(&self) -> Option<&ChannelID>;
  fn channel_data(&self) -> Option<&ChannelData>;
}

impl ChannelMessage for MessageStandard {
  fn channel_id(&self) -> Option<&ChannelID> {
    &self.channel
  }

  fn channel_data(&self) -> Option<&ChannelData> {
    None
  }
}

impl ChannelMessage for ChannelCreated {
  fn channel_id(&self) -> Option<&ChannelID> {
    self.channel.as_ref().map(|c| c.id)
  }

  fn channel_data(&self) -> Option<&ChannelData> {
    &self.channel
  }
}