Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change field visibility and single-expression return syntax #335

Merged
merged 3 commits into from
Jul 3, 2021

Conversation

NoakPalander
Copy link
Contributor

  • Changed the visibility of
    dev.kord.rest.builder.interaction.OptionsBuilder.name, .description, .type from public to protected due to them serving no purpose as public from an API standpoint. E.g
guild.createApplicationCommand("...", "...") {
    string(name="some_name", description="some_description") { // obligatory fields
        this.name = "some_other_name" //  this line serves no purpose
        this.description = "some_other_description" // this line serves no purpose
        this.type = ApplicationCommandOptionType.String // this is already specified by the `string` choice-builder
    }
}

The reason for them not being private is due to BaseCommandOptionBuilder.toRequest and BaseChoiceBuilder.toRequest requiring access to these fields when returning an ApplicationCommandOption object.

  • Changed the .toRequest functions to single expression return syntax
    Reduces some redundancy, e.g return type and unnecessary return keyword

…ed the toRequest functions to single expression-styled returns
@HopeBaron
Copy link
Member

HopeBaron commented Jun 29, 2021

Hello! Thanks for your contribution
While I do agree that type should be private in order to eliminate unintended behavior, name and description should still be public, applying logic to your paramters becomes kind of annoying, some people would like to change name/description while they are building the command.

@NoakPalander
Copy link
Contributor Author

NoakPalander commented Jun 29, 2021

Created a new commit. How would you feel about having the two remaining fields as nullable and default initialized. When applying "logic" to them one would still have to specify the fields and assign them empty strings as:

string("", "") {
    name = foo()
    description = bar()
}

while that could be changed to e.g

string { // name: String? = null
    name = foo()
}

Whilst still allowing the previous

string(name, description)

@HopeBaron
Copy link
Member

Thanks for your question.
The fact that they are mandatory is that they are required.
it's required to have a name and description in the Discord system, they can't be null.
but Kord allows you to "edit" them before you send the final request.

what would be an appropriate representation of null in a parameter that is required? an empty string? some default string?
The type reflects the requirement by Discord as well, not only that of Kord's.

@NoakPalander
Copy link
Contributor Author

I can see how that's be problematic. Anyhow, I pushed another commit, please review if it's suitable for a merge :)

@HopeBaron
Copy link
Member

@NoakPalander sorry for the delay on the merge. can you make type public but make it a val instead?
this would allow the user to check for the argument type inside an OptionBuilder of an unknown type

@HopeBaron
Copy link
Member

Thank you for your contribution! Hit us on Discord if you haven't!

@HopeBaron HopeBaron merged commit 9258d3c into kordlib:0.7.x Jul 3, 2021
HopeBaron added a commit that referenced this pull request Jul 8, 2021
* Add a cache aware rest entity supplier (#338)

* add a cahe aware rest supplier/strategy

* abstract the new flow creation for storeAndEmit

* apply suggestions

* Reduce the number of iterations

rename storeAndEmit to storeOnEach

* update changelog

* Fix master gateway average ping conversion (#337)

* Change field visibility and single-expression return syntax (#335)

* changed the visibility for the name, descript.. and type field, changed the toRequest functions to single expression-styled returns

* remade .name and .description to public upon request, kept .type protected

* remade .type into read only (val)

* Implement select menus (#339)

* Add select menu json representation

* Add select menu builders

* add serialization tests for select menu interactions

* Add core implementation of select menus

* Update ButtonComponent docs

* Make SelectMenu min/max values a range

* Fix getGuildBanOrNull recursion (#341)

Fixes the self call in getGuildBanOrNull by delegating the call to the supplier instead

* Make UpdateMessageInteractionResponseCreateBuilder fields nullable (#340)

Makes the fields in the builder and request nullable where appropriate for updating a message

* improve readme

* api dump

* add breaking changes to changelog

* mention the type visibility change

[ci skip]

Co-authored-by: Bart Arys <[email protected]>
Co-authored-by: Noak Palander <[email protected]>
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.

2 participants