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

feat(twilight-model,twilight-validate)!: Implement select menu default values #2281

Merged

Conversation

archer-321
Copy link
Contributor

@archer-321 archer-321 commented Sep 28, 2023

This pull request adds the default_values field Discord recently introduced for select menus. This field allows selecting default values for auto-populated select menu types (currently, user, role, mentionable, and channel).

Setting this field to Some(...) for any other select menu type is an error at runtime. However, I've changed the twilight-verify implementation to detect mistakes like this before sending the data to Discord.

Given the previous decision to implement select menu types as entirely flattened structs, I added the default_values field as an option to SelectMenu. While this doesn't statically enforce the type requirement explained above, this matches the behaviour of other type-specific fields.

Testing

I've updated my simple test project to use the feature introduced by this PR. As with my last PR, you can use it to get a quick overview of how the new API works.

Breaking changes

I'm aware of the following breaking change that this PR introduces:

  • SelectMenu (exhaustive) received a new default_values field.

I've updated the documentation and tests wherever necessary, but if I missed something or you want me to rephrase a comment, please request changes.

I believe you should add the d-api and d-breaking labels to this PR.

@github-actions github-actions bot added c-model Affects the model crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature labels Sep 28, 2023
archer-321 added a commit to archer-321/twilight-select-menus that referenced this pull request Sep 28, 2023
This commit updates the README to mention the second pull request using
this project (twilight-rs/twilight#2281). Moreover, it removes the
references to the new removed `!select` text command.
@archer-321
Copy link
Contributor Author

Interestingly enough, the test passed with the (outdated) version I had in my Cargo.lock file.
After a quick cargo update, I could reproduce the failure and push a fix.

Copy link
Member

@laralove143 laralove143 left a comment

Choose a reason for hiding this comment

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

thank you for this pr, i just added some test requests and nits

twilight-model/src/channel/message/component/mod.rs Outdated Show resolved Hide resolved
twilight-validate/src/component.rs Outdated Show resolved Hide resolved
twilight-validate/src/component.rs Show resolved Hide resolved
laralove143
laralove143 previously approved these changes Nov 1, 2023
Copy link
Member

@laralove143 laralove143 left a comment

Choose a reason for hiding this comment

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

thank you for this implementation :)

@laralove143 laralove143 dismissed their stale review November 1, 2023 00:33

need clarification on tests

@archer-321
Copy link
Contributor Author

@laralove143, I could reproduce the CI failure locally (after setting the necessary environment variables).
However, I believe this failure is unrelated to this PR, as the main branch seems to be failing with the same error.
Maybe this was introduced by better/worse detection of unused exports in a nightly toolchain update? 🤔

@archer-321
Copy link
Contributor Author

I believe the CI error this PR is experiencing will be fixed by #2295.
I'll rebase the source branch of this PR once 2295 is merged 🙂

…t values

This commit implements the default_values field for auto-populated
select menu components. Currently, it's supported by user-, role-,
mentionable-, and channel select menus.

In addition to adding the `SelectDefaultValue` type and its field
to `SelectMenu`, this commit updates the existing tests to also test
this new feature. Moreover, it updates twilight-validate to validate
the invariants specified by Discord's API reference.
This commit updates the component unit tests to use `UnitVariant`
instead of `Str` for the adjacently tagged `SelectDefaultValue` enum.

Introduced by a SemVer-compatible update of the `serde` crate, this
implementation detail was enough to break this unit test when using
an up-to-date `Cargo.lock`.
This commit implements select menu serialization tests. With most parts
of (text) select menus already tested by `component_full`, this commit
focuses on the `default_values` field.

In addition to updating the select menu component tests, this patch
implements Copy for `SelectMenuType`. Moreover, it moves the individual
validation parts for the `default_values` field to separate functions.
Those functions then received unit tests. Overall, this design matches
the rest of the component validation code.
This commit removes all tokens related to the default_values field
from the `component_full` test. The previous commit set this field to
`None` but didn't update the expected tokens.

With this commit, all tests should pass again.
This commit changes the inner `check_select` function in
`message::component::tests::select_menu` to take a vector of select
menu default values *and* the respective &'static str. This allows
removing the previous Box::leak that was necessary to convert a
dynamically generated string into a 'static str slice.
@archer-321 archer-321 force-pushed the feat/select-menu-default-values branch from e33f30f to f07f652 Compare November 4, 2023 20:42
@archer-321
Copy link
Contributor Author

I rebased the PR, and this fixed the CI. Please let me know if there are any other changes you'd like to see c:

Copy link
Member

@laralove143 laralove143 left a comment

Choose a reason for hiding this comment

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

thank you for the implementation!

@laralove143 laralove143 merged commit 7e58f89 into twilight-rs:next Nov 7, 2023
@archer-321 archer-321 deleted the feat/select-menu-default-values branch November 7, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-model Affects the model crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants