Add the rest of the attachment fields to Attachment struct

[mcasper]

Support the rest of the attachment fields detailed here: https://api.slack.com/docs/message-attachments#attachment_structure

[compressed]

Should be Option<String> because we don't need to do any escaping in the URL. We should send that as is (e.g. & shouldn't be changed to & in a URL).

[compressed]

Thanks @mcasper! One minor change here: you don't need to escape the links sent to slack, only the text.

[mcasper]

Oh good point, thanks @compressed! Do we want those fields to be Option<&' str> though, to stay consistent with the rest of the struct? Do we want to support Option<Into<String>>?

[compressed]

Well Attachment needs to own its data, so storing a &str there won't be very useful...

I think you mean to be consistent with AttachmentTemplate, in that case, you're right those would stay as Option<&'a str> or I'd be okay changing those into Option<Into<String>> as it's a bit more flexible.

However, I was also thinking that we should really avoid String and use Url instead since rust-url now exists. It already has a Encodable impl as well (need to opt in rustc-serialize feature). We'd then need an IntoUrl trait like hyper does (http://hyper.rs/hyper/v0.9.9/src/hyper/src/client/mod.rs.html#429-432) which we can use for the template type and then use try! when building the actual attachment.

If you're up for it, I'd like to get to that point. If not, you can just keep it as String and I'll make those changes in the next few days. This library is in need of a little refactoring :)

[compressed]

Oh one more thing, please add:

[breaking-change]

to the end of your commit message.

[mcasper]

I'm absolutely up for it!

The current method of building payloads/attachments through template structs is at war with the thought to use an IntoUrl trait, as we can't specify Option<IntoUrl> for the url fields. How would you feel about using a builder pattern to clean up field building?

let attachment = Attachment::new() // initializes with all default values
    .text("Attachment text")
    .color("#6800e8")
    // etc

let payload = Payload::new()
    .attachments(vec![attachment])
    .channel("#channel")
    // etc

This would allow all of the url builder functions to accept U: IntoUrl, would remove the need for users to specify a lot of fields to change just one or two, and would remove the need for template enums

[compressed]

Awesome! Yup, I was thinking that the template is not really a good choice anymore and going to builder pattern is more flexible. I'd like to move forward with that approach.

However, there is a challenge with using the builder pattern and handling the error ergonomics. Currently, making an attachment (all at once) will return a Result since it can fail (HexColor), but now we'll also have other things that can fail to be created (Urls). With the builder, setting any of the links could fail, so I believe those builder methods need to return a Result where the operation could fail.

I think your example would look more like this (using the question mark notation, which should arrive at some point...):

let attachment: Result<Attachment, SlackError> = Attachment::default()
    .text("Attachment text")?
    .color("#6800e8")?
    .author_link("http://example.com")?
    .author_icon("http://example.com/icon");

which isn't too bad, but with try! as we have now, this makes it quite messy to use...

So onto another approach... We can use the builder strategy as mentioned here: colin-kiegel/rust-derive-builder#25 (comment).

We'd then have something like this:

let attachment: Result<Attachment, SlackError> = AttachmentBuilder::new()
    .text("Attachment text")
    .color("#6800e8")
    .author_link("http://example.com")
    .author_icon("http://example.com/icon")
    .build();

which seems like it will work and seems better for usability.

The underlying implementation would something like this:

struct AttachmentBuilder {
    inner: Result<Attachment, Error>,
}

impl AttachmentBuilder {
    fn new() -> AttachmentBuilder {
        AttachmentBuilder { inner: Ok(Default::default()) }
    }

    fn fallback<S: Into<String>>(mut self, text: S) -> AttachmentBuilder {
        match self.inner {
            Ok(mut inner) => {
                inner.fallback = SlackText::new(text); // I will refactor this to accept `Into<String>`
                AttachmentBuilder { inner: Ok(inner) }
            }
            _ => self,
        }
    }

    ....

    fn build(self) -> Result<Attachment, Error> {
        self.inner
    }
}

Let me know your thoughts. I could see a macro being useful here to help write the methods, since most of them will look quite similar.

Since this will be a breaking change, I'm going to take this opportunity to do some other refactoring of the library, especially cleaning up the Error / Result naming.

[homu]

☔ The latest upstream changes (presumably #8) made this pull request unmergeable. Please resolve the merge conflicts.

[compressed]

@mcasper I've completed the refactoring I wanted to do. You can check this branch: https://github.com/compressed/rust-slack/tree/builder. The code should be much cleaner now. I've added starting builder functions for Payload and Attachment and it seems to be working quite well using the strategy I discussed above.

Please make your changes on top of my branch and then I can review and pull them in.

One other point: I've added the TryFrom and TryInto traits from rust proper since they haven't stabilized (yet: https://doc.rust-lang.org/std/convert/trait.TryFrom.html). You can use these to set the bound for the "url fields" as TryInto<URL>. Check out HexColor for an example of how I did it.

Let me know if you have any questions. Thanks!

[compressed]

@mcasper I went ahead and added these fields (along with ts) with the rest of the refactoring changes. Thanks!