Skip to content

Conversation

@DRSchlaubi
Copy link
Member

No description provided.

Copy link
Member

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

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

review of common

/**
* Adds a separator with specified [spacing].
*/
public fun separator(spacing: SeparatorSpacingSize): Unit = separator {

This comment was marked as resolved.

/**
* Adds a separator with specified [spacing].
*/
public fun separator(spacing: SeparatorSpacingSize): Unit = separator {
Copy link

Choose a reason for hiding this comment

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

Suggested change
public fun separator(spacing: SeparatorSpacingSize): Unit = separator {
public fun separator(
spacing: SeparatorSpacingSize? = null,
divider: Boolean? = null,
): Unit = separator {
this.spacing = spacing
this.divider = divider
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really see the point of that, you can just call the seperator function with the builder yourself

Copy link

Choose a reason for hiding this comment

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

I agree, but there is no point to have partial member function. My opinion is "all or nothing" :D

@ptitjes
Copy link

ptitjes commented May 11, 2025

Hey @DRSchlaubi ! Nice PR. I had to make a few tiny changes to make it work here:

  • in the common module, regenerate code and dump API again
  • in the rest module, dump API again
  • make DiscordComponent.description both Optional and nullable [1]

You can see the details in the last three commits here:
https://github.com/ptitjes/kord/commits/feat/components-v2-didier/

[1] When deserializing the incoming messages in the Gateway, the description field sent back by Discord was null, and this was creating a deserialization error.

gdude2002

This comment was marked as outdated.

@lukellmann
Copy link
Member

I haven't had time to look into things in detail yet, but the presence of AbstractMessageCreateBuilder and AbstractMessageModifyBuilder is a strong indictment of Kord's development philosophy, imo.

These types aren't new and weren't even touched in this PR, they have been there since #891

This approach makes no sense from a logical viewpoint, and will significantly complicate things for API consumers - now instead of having a single base type that already worked everywhere, I need to mix between the interface and abstract type depending on whether I want to use components or not, and I need to go back over every single piece of code that currently uses the interface to make sure it doesn't now actually need the abstract type.

I couldn't find anything you can do with the Abstract* classes that you can't do with the interfaces. Did you need to use the Abstract* classes in some code? You shouldn't have to.

* A message can have up to five action rows.
* A message can have up to ten top-level components.
*/
@Deprecated("Use ComponentContainerBuilder#actionRow instead.")
Copy link
Member

@lukellmann lukellmann Jul 2, 2025

Choose a reason for hiding this comment

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

This deprecation isn't actionable since this extension is more specific than ComponentContainerBuilder.actionRow (MessageBuilder implements that interface). Just don't deprecate it or use @LowPriorityInOverloadResolution. Should also add replaceWith and explicit level when going with the deprecation.

@nillpoe nillpoe mentioned this pull request Aug 21, 2025
@Galarzaa90
Copy link
Contributor

Is there something that can be helped with regarding this PR?

Would love to see this implemented.

@DRSchlaubi
Copy link
Member Author

#1019, #1018 need fixing

@NotJansel
Copy link

NotJansel commented Sep 29, 2025

tested around a bit and so far there was no error during the entire testing that could have been related to components v2

Edit: found out the hard way that a section cant have more than 3 text displays.

Copy link
Contributor

@NoComment1105 NoComment1105 left a comment

Choose a reason for hiding this comment

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

Realised I never actually read this through, Looks good to me, one comment from Luke about deprecation that I agree with but other than that perfect :D

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.