search

slackapi / java-slack-sdk

Slack Developer Kit (including Bolt for Java) for any JVM language
https://tools.slack.dev/java-slack-sdk/
MIT License
579 stars 216 forks source link

Add optional Kotlin builders for building messages #428

Closed emanguy closed 4 years ago

emanguy commented 4 years ago

Issue Type

Description

I'd like to be able to build slack messages in more of a "kotlin builder" style rather than the functional chain builders that are currently available. I'm also willing to contribute the code if need be, just want to add an issue here.

For example, here's how building a slack message currently looks in Kotlin code:

slack.methods(token).chatPostMessage { req -> 
  req.channel("C1234567")
  .blocks(asBlocks(
    section { section -> section.text(markdownText("*Please select a restaurant:*")) },
    divider(),
    actions { actions -> 
      actions.elements(asElements(
        button { b -> b.text(plainText(pt -> pt.emoji(true).text("Farmhouse"))).value("v1") },
        button { b -> b.text(plainText(pt -> pt.emoji(true).text("Kin Khao"))).value("v2") }
      ))
    }
  ))
);

If we were to use a kotlin builder style, we could do something like this instead, which fits in with the language a little better:

slack.methods(token).buildAndPostMessage {
  channel = "C1234567"
  blocks {
    section {
      markdownText("*Please select a restaurant:*")
    }
    divider()
    actions {
      elements {
        button {
          plainText(text = "Farmhouse", emoji = true, value = "v1")
        }
        button {
          plainText(text = "Kin Khao", emoji = true, value = "v2")
        }
      }
    }
  }
}

Requirements (place an x in each of the [ ])

seratch commented 4 years ago

@emanguy Thanks for taking the time to write in! The suggested API looks neat. One thing I'd like to know more is how the implementation looks like. Is it possible to ask you to show proof of concept code (you don't need to support many APIs)?

If it continuously requires efforts for future maintenance, I may be a bit reluctant to add the support.

emanguy commented 4 years ago

Okay, I'll go ahead and throw some sample code into a pull request and reference here.

emanguy commented 4 years ago

Alright, I've made a fork of the slack library. I added a new Maven module for the Kotlin builders in the directory slack-api-client-kotlin-builders on the kotlin-dsl branch. The fork can be found here.

I tried to keep the UX of the DSL similar to that of kotlinx.html where basic data type attributes are filled in via optional parameters on function invocations and more complex data is built inside the lambda body. Some of the benefits of this DSL's design are:

image

Cons of course are:

Tests demonstrating sample usage and validation can be found here. Looking forward to hearing your thoughts!

seratch commented 4 years ago

@emanguy Wow, you're so quick! I will check it out today.

seratch commented 4 years ago

@emanguy First of all, thank you very much for taking the time to create this PoC. The implementation is greatly inspiring and is already great enough.

But I have a few things to change or discuss. I've modified your branch this way. Could you check it out when you have a chance? https://github.com/seratch/java-slack-sdk/commit/30c68b4b17829c718899ffa40c2896ebd5c2fae6

The following are my thoughts about the possibility to have this as a 1st-party optional module.

If you're comfortable with these, I'd love to start working with you on this. Even if you don't have time to work on this in the short-term, I can complete all the work on my own within a certain period. As I'm in charge of other SDKs, I don't commit a concrete schedule at this point, though.

By any chance, if you're not happy with the above, of course, it's also great to create the module as your personal project. Feel free to take some of my changes for it in the case.

emanguy commented 4 years ago

Yeah, I have tons of time on weekends to keep working on this. Looking forward to working with you more on this!

I'm fine with constraining the scope to the blocks, but do you think it would be possible to add kotlin extension functions which would just allow developers to invoke a .buildBlocks() function off of builders such as the ChatPostMessageRequest lombok builder or is that too far outside the scope?

I'll take a deeper look into your modifications soon. Thanks for the feedback!

seratch commented 4 years ago

@emanguy

kotlin extension functions

I'm fine with this idea! (needless to say to you though, as long as we don't abuse in unnecessary use cases)

I was just concerned about having almost the same structure with all the request classes. As Slack already has 180+ documented endpoints, it's unrealistic to maintain doubled ones without missing anything.

Regarding the project structure idea, it's possible to have those functions and DSLs for blocks in one maven module as you did but I'd like to separate those into slack-api-model-kotlin-extention and slack-api-client-kotlin-extension for clarity. The client extension can depend on the model extension.

Good to know you're happy with the above so far!

emanguy commented 4 years ago

Would you like me to remove the existing built-in validation or should I leave it?

Also, I found a much better way to handle the delegation through Kotlin's built-in delegation. Makes it so we don't have to repeat the interface methods over and over again. I'll be using this moving forward:

@BlockLayoutBuilder
class ActionsBlockBuilder private constructor(
        private var blockId: String?,
        private val elementsContainer: MultiBlockElementContainer
) : Builder<ActionsBlock>, BlockElementDsl by elementsContainer {

    constructor(blockId: String?) : this(blockId, MultiBlockElementContainer())

    override fun build(): ActionsBlock {
        return ActionsBlock.builder()
                .blockId(blockId)
                .elements(this.elementsContainer.underlying)
                .build()
    }
}

This way, the compiler just generates the public members of the interface for us automatically. You can read more about this in the Kotlin docs here

seratch commented 4 years ago

@emanguy Nice! > property delegation by Kotlin language

Would you like me to remove the existing built-in validation or should I leave it?

I prefer removing it so far. If we can provide some ways to enable users to add their own rules, that would be great (I don't have any good idea yet, though).

emanguy commented 4 years ago

Looking over some of your changes, it looks like you included what appear to be setter functions on the button builder for the actionId, url, and value fields on the button. Should we be doing this everywhere? I originally designed this so that fields with "basic" data types (i.e. ints, strings, enums, etc.) would be filled in builder function parameters, while nested objects (i.e. PlainTextObject, ImageElement, ConfirmationDialogObject, etc.) would be constructed within the lambdas.

Is there a reasoning behind this? I'd prefer to go the original route but you're the library maintainer so I'll defer to you.

EDIT: If you want to continue setting those after the fact, we could just make them public vars so the properties are still accessible and editable

seratch commented 4 years ago

@emanguy

I've updated my branch a bit. I'd like to keep the DSL as similar to JSON (in terms of its structure) as possible. So, one possible way is having var arguments in constructors plus providing setter methods this way.

The usage of the DSL looks like this: https://github.com/seratch/java-slack-sdk/blob/slack-api-model-kotlin-extension/slack-api-model-kotlin-extension/src/test/kotlin/test_locally/BlockKitBuilderTest.kt#L25-L43

I'm sure there is no confusion for library users with this.

.blocks(withBlocks {
  section {
    blockId("section-block-id")
    plainText("This is the text in this section")
  }
  divider()
  actions {
    blockId("block-id")
    button {
      actionId("action-id-value")
      url("https://www.google.com")
      style(ButtonStyle.PRIMARY)
      plainText("Go to Google")
      confirm {
        title("Confirm Navigation")
        markdownText("Are you *absolutely sure* you want to go to google?")
        confirm("Yes, let's go!")
        deny("I'm not sure...")
      }
    }
  }

I'm fine to have your original idea too. However, I'm a bit concerned it may be unclear and confusing for developers to use the method arguments only for basic types while using the function for custom classes. 

button(
  actionId = "action-id-value",
  url = "https://www.google.com",
  style = ButtonStyle.PRIMARY
) {
  plainText("Go to Google")
  confirm {
    title("Confirm Navigation")
    markdownText("Are you *absolutely sure* you want to go to google?")
    confirm("Yes, let's go!")
    deny("I'm not sure...")
  }
}

So, if we provide such ways, I feel doing the following way should be also allowed. 

button(
  actionId = "action-id-value",
  url = "https://www.google.com",
  style = ButtonStyle.PRIMARY,
  text = plainText("Go to Google"),
  confirm = confirm {
    title("Confirm Navigation")
    markdownText("Are you *absolutely sure* you want to go to google?")
    confirm("Yes, let's go!")
    deny("I'm not sure...")
  }
)
emanguy commented 4 years ago

I don't want to allow for any sort of nesting in the parameters, so I'd rather not allow for adding complex data (i.e. types other than things like int, double, float, string, etc.) in the parameters to these builder functions. However, I do understand that there may be confusion in having to set these in a different location, so here's my current thought:

On the builders, we can make the constructor inputs for these basic data types public vars, so within the body of a button builder for instance you could write something like this:

button {
  actionId = "action-id-value"
  url = "https://www.google.com"
  style = ButtonStyle.PRIMARY
  // etc.
}

which would be equivalent to this:

button(
  actionId = "action-id-value",
  url = "https://www.google.com",
  style = ButtonStyle.PRIMARY
) {
  // etc.
}

The reason I designed the DSL the way I originally did was because I saw basic data types as "properties" of the blocks we were building, while more complex data felt more like the "content" of the block, similar to the way kotlinx.html represents the properties and content of an HTML tag:

<div class="myclass otherclass" id="thediv">
  <p>This is my text!</p>
</div>
div(class = "myclass otherclass", id = "thediv") {
  p {
    +"This is my text!"
  }
}

It looks like kotlinx.html does something similar within the body with its parameter attributes, so it should be familiar enough to Kotlin devs: https://github.com/kotlin/kotlinx.html/wiki/Elements-CSS-classes#css-classes

Additionally, by declaring the builder constructor inputs as public vars, we don't have to define separate setter functions which means less maintenance work.

seratch commented 4 years ago

Indeed, your example looks very simple.

button {
  actionId = "action-id-value"
  url = "https://www.google.com"
  style = ButtonStyle.PRIMARY
  // etc.
}

However, what do you think about this?

actions {
  blockId = "block-id"
  button {
    actionId = "action-id-value"
    url = "https://www.google.com"
    style = ButtonStyle.PRIMARY
    plainText("Go to Google")
    confirm {
      title("Confirm Navigation")
      markdownText("Are you *absolutely sure* you want to go to google?")
      confirm("Yes, let's go!")
      deny("I'm not sure...")
    }
  }
}

I don't think this is easy enough to compose a valid structure without any errors. The necessity to understand the difference between actionId and text/confirm looks almost unacceptably unintuitive to me.

If we choose your approach, I know this is a bit more verbose but the following one looks more consistent to me. If you meant this, this is totally fine for me too although I still want to have the setter methods I mentioned above as well.

actions {
  blockId = "block-id",
  button = button {
    actionId = "action-id-value",
    url = "https://www.google.com",
    style = ButtonStyle.PRIMARY,
    text = plainText("Go to Google"),
    confirm = confirm {
      title = title("Confirm Navigation"),
      text = markdownText("Are you *absolutely sure* you want to go to google?"),
      confirm = confirm("Yes, let's go!"),
      deny = deny("I'm not sure...")
    }
  }
}

Additionally, by declaring the builder constructor inputs as public vars, we don't have to define separate setter functions, which means less maintenance work.

Regarding my idea where every single property has its setter method (say, blockId(String)), I can handle the efforts even for the future as long as it covers only Block Kit UI components.

similar to the way kotlinx.html represents the properties and content of an HTML tag:

I do understand this point where we should follow a familiar way for Kotlin devs.

However, Slack's Block Kit is not an HTML data structure. Its JSON structure is relatively simple. From that perspective, I'm not so convinced to follow the exact same concept yet. JSON doesn't have a complex structure such as properties and body value for a tree element.

I really don't want to discourage you but this is a really important design decision for future maintenance. So, I would like to be a bit careful about it. I would appreciate it if you could understand it.

emanguy commented 4 years ago

Okay, this all makes sense. I'm leaning more towards the design you suggested previously as it is less repetitive than actionId = ..., text = ..., etc.:

actions {
  blockId("block-id")
  button {
    actionId("action-id-value")
    url("https://www.google.com")
    style(ButtonStyle.PRIMARY)
    plainText("Go to Google")
    confirm {
      title("Confirm Navigation"),
      markdownText("Are you *absolutely sure* you want to go to google?"),
      confirm("Yes, let's go!"),
      deny("I'm not sure...")
    }
  }
}

Should I move forward with this design or should we refine it further? Apologies for trying to take the design in an unfavorable direction for so long.

seratch commented 4 years ago

Thanks for your understanding.

Should I move forward with this design or should we refine it further?

I think we can move it forward with the design. If we find further topics after starting its implementation, let's use this issue to discuss those.

If you create a WIP pull request for this task, I also can submit suggestions or partial changes towards your fork's branch 💪

emanguy commented 4 years ago

The first draft of the DSL and client extensions are tentatively done, I think I just need to add some more extensive testing and doc comments.

One addition I made that was slightly off-script was within the scope of the Rich Text Element builders. I added some simple overloads for some rich text elements for the case where you just want to add things like emoji or text but don't want to add a full builder block, as seen in the API client extension module here: https://github.com/emanguy/java-slack-sdk/blob/f3a74aa8a18a2dcc8af12b68ed61867f244d5c0b/slack-api-client-kotlin-extension/src/test/kotlin/test_locally/api/methods/kotlin_extension/request/chat/ExtensionMethodsTest.kt#L19

Looking forward to your review!

seratch commented 4 years ago

@emanguy Thanks for your amazing work in the PR! It already looks so great. Can you check the following?

emanguy commented 4 years ago

Hey there! I just added a bunch of tests which brings our class coverage up to 100% and our method coverage to 75%. I think the code is ready for review and merge, if possible. Thanks for all the feedback!

seratch commented 4 years ago

As the remaining documentation task will be tracked by #501 , let me close this issue now.

Before closing, let me repeat the same comment here - I'm really glad to have such amazing modules in this SDK 🎉 Thanks a lot for your great efforts 🙇