Add a builder for CallbackData #1146

baptiste0928 · 2021-09-01T16:08:40Z

This PR adds a builder for CallbackData. Similar design to the other builders in the model crate. I needed to add some #[allow(clippy::missing_const_for_fn)] to ignore Clippy false positives.

Closes #976

model/src/application/callback/callback_data.rs

vilgotf

Also on all methods, separate the assignment self.field = Some(value); and returning self with a blank line.

baptiste0928 · 2021-09-01T17:07:05Z

@vilgotf I resolved your review comments

vilgotf

Last thing: add an assert_impl_all! and verify Debug, Default, Send, Sync

AEnterprise · 2021-09-02T06:38:53Z

model/src/application/callback/callback_data.rs

+        self._content(content.into())
+    }
+
+    #[allow(clippy::missing_const_for_fn)]


Is there a reason this isn't const?

The compilation fails if the function is const. I don't know much about const fns, maybe there is a way to fix it properly.

Even if this was const, wouldn't it not matter as content itself can't be const?

vilgotf

I think this is blocking on #1056 tho

7596ff · 2021-09-30T14:58:21Z

This can now be updated, please put the builder in the util/builder module.

7596ff

thanks for the changes, a few things:

util/src/lib.rs

util/src/builder/mod.rs

util/src/builder/callback_data.rs

7596ff · 2021-09-30T18:48:11Z

util/src/builder/callback_data.rs

+/// # Example
+/// ```
+/// use twilight_util::builder::CallbackDataBuilder;
+/// use twilight_model::channel::message::MessageFlags;
+///
+/// let callback_data = CallbackDataBuilder::new()
+///     .content("My callback message")
+///     .flags(MessageFlags::EPHEMERAL)
+///     .build();
+/// ```


Can we move this example to a module-level doc comment?

Do you want to move it to the builder module documentation? As the callback_data module contains only one struct, I thought that re-exporting the type directly to the root of the builder and making callback_data module private was better.

I mean in the same file, just having it at the top with //! comments. See cache/in-memory/src/permission.rs for an example

Which appears like this: https://api.twilight.rs/twilight_cache_inmemory/permission/index.html

Yes, but actually the util::builder::callback_data is not exported, only the CallbackDataBuilder type is re-exported in the builder module. Do you want to export the callback_data module entirely?

I'll have to get back to you on this, I'll pull in the code and play around with how it renders, then let you know.

Co-authored-by: Cassandra McCarthy <cassie@7596ff.com>

7596ff

These are all just documentation nits.

util/src/builder/callback_data.rs

7596ff · 2021-10-01T14:59:54Z

I've played around with it. I think for this builder and the command builder, I feel it would be best to respect the clippy::module_name_repetitions lint, name the primary builder of the module Builder, and allow users to rename it on import. This should be discussed though. What do y'all think?

Co-authored-by: Cassandra McCarthy <cassie@7596ff.com>

resolved

baptiste0928 · 2021-10-01T15:12:58Z

I think that naming the type Builder will be confusing for most users. This will also make it harder to discover builders, as the search bar will become less relevant, and it will not be clear whether a builder is available for a given type.

"Best practices" enforced by Clippy are great to ensure an overall quality of the code, but should not make code harder to understand by developers using the library. It's why I think exporting builders at the root of the crate is actually the best option to make them easily discoverable, as we can have at a glance an overview of all available builders.

Furthermore, I think that it could be great to introduce a feature in the model crate to add builder() methods on types that provide a builder. This will at least show that a builder is available in docs.rs, and will make them easier to discover.

7596ff · 2021-10-01T15:20:00Z

After referring back to #1056, I realize my proposal conflicts with the other proposed design, so we'll just scrap it. I also agree with your first two paragraphs, so thank you for writing them.

Furthermore, I think that it could be great to introduce a feature in the model crate to add builder() methods on types that provide a builder. This will at least show that a builder is available in docs.rs, and will make them easier to discover.

As for discoverability, we are actually meant to link to the builders on their respective types in model, so I'd like it if we did that instead of making a new feature on model at this time.

vilgotf

Could you also alphabetize the methods of CallbackDataBuilder & add components to the example?

util/src/builder/callback_data.rs

Co-authored-by: Vilgot Fredenberg <vilgot@fredenberg.xyz>

vilgotf

sorry, the methods were already good 😅, could you revert that change?

new
build
(rest of the methods in alphabetical order)

baptiste0928 · 2021-10-02T14:08:37Z

No problem, changes reverted. (oops I wrote half of the commit message in French)

vilgotf

Clippy error is irrelevant

7596ff

one more thing

util/src/builder/callback_data.rs

This PR adds a builder for [`CallbackData`](https://docs.rs/twilight-model/0.6.2/twilight_model/application/callback/struct.CallbackData.html). Similar design to the other builders in the `model` crate. I needed to add some `#[allow(clippy::missing_const_for_fn)]` to ignore Clippy false positives. Closes twilight-rs#976. Co-authored-by: Cassandra McCarthy <cassie@7596ff.com> Co-authored-by: Vilgot Fredenberg <vilgot@fredenberg.xyz>

@vilgotf

Adds the `builder` module, which is currently populated with the `CommandBuilder` ([#1048] - [@vilgotf]) and the `CallbackDataBuilder` ([#1146] - [@baptiste0928]). [#1048]: #1048 [#1146]: #1146 [@baptiste0928]: https://github.com/baptiste0928 [@vilgotf]: https://github.com/vilgotf

vilgotf requested changes Sep 1, 2021

View reviewed changes

model/src/application/callback/callback_data.rs Outdated Show resolved Hide resolved

model/src/application/callback/callback_data.rs Outdated Show resolved Hide resolved

model/src/application/callback/callback_data.rs Outdated Show resolved Hide resolved

vilgotf requested changes Sep 1, 2021

View reviewed changes

vilgotf requested changes Sep 2, 2021

View reviewed changes

AEnterprise reviewed Sep 2, 2021

View reviewed changes

baptiste0928 requested a review from vilgotf September 2, 2021 15:15

vilgotf previously approved these changes Sep 2, 2021

View reviewed changes

zeylahellyer added c-model Affects the model crate t-feature Addition of a new feature f-application-commands labels Sep 5, 2021

zeylahellyer marked this pull request as draft September 5, 2021 16:02

baptiste0928 added 5 commits September 30, 2021 19:46

Create CallbackDataBuilder

30776fe

Re-export the builder in parent module

55ffb6a

Apply requested changes

fb78662

Add missing newlines before returning self

0617e56

Add assert_impl_all test

9740a3a

baptiste0928 dismissed vilgotf’s stale review via 9740a3a September 30, 2021 18:42

baptiste0928 force-pushed the callback-data-builder branch from 736cdba to 9740a3a Compare September 30, 2021 18:42

Move builder to util crate

7316f80

baptiste0928 marked this pull request as ready for review September 30, 2021 18:44

7596ff suggested changes Sep 30, 2021

View reviewed changes

Requested changes

c37b5b6

Co-authored-by: Cassandra McCarthy <cassie@7596ff.com>

7596ff previously requested changes Oct 1, 2021

View reviewed changes

Documentation improvements

eea8f55

Co-authored-by: Cassandra McCarthy <cassie@7596ff.com>

vilgotf requested changes Oct 1, 2021

View reviewed changes

baptiste0928 and others added 2 commits October 2, 2021 15:12

Apply suggestions from code review

592de08

Co-authored-by: Vilgot Fredenberg <vilgot@fredenberg.xyz>

Alphabetize methods and improve example

eb95ff5

baptiste0928 requested a review from vilgotf October 2, 2021 13:28

vilgotf requested changes Oct 2, 2021

View reviewed changes

Reorder methodes

900a81c

Use new in defaut method

e4c0b23

vilgotf previously approved these changes Oct 2, 2021

View reviewed changes

7596ff suggested changes Oct 2, 2021

View reviewed changes

util/src/builder/callback_data.rs Outdated Show resolved Hide resolved

Test Clone implementation

96e1f4b

baptiste0928 dismissed vilgotf’s stale review via 96e1f4b October 2, 2021 15:41

baptiste0928 requested review from vilgotf and 7596ff October 2, 2021 15:42

vilgotf approved these changes Oct 2, 2021

View reviewed changes

7596ff approved these changes Oct 2, 2021

View reviewed changes

7596ff merged commit 98e97de into twilight-rs:main Oct 2, 2021

baptiste0928 deleted the callback-data-builder branch October 2, 2021 18:23

vilgotf mentioned this pull request Oct 24, 2021

Tracking issue: Slash Commands #796

Closed

7 tasks

zeylahellyer added this to the Builder Restructuring milestone Dec 26, 2021

zeylahellyer assigned baptiste0928 Dec 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a builder for CallbackData #1146

Add a builder for CallbackData #1146

baptiste0928 commented Sep 1, 2021

vilgotf left a comment

baptiste0928 commented Sep 1, 2021

vilgotf left a comment

AEnterprise Sep 2, 2021

baptiste0928 Sep 2, 2021

PyroTechniac Sep 2, 2021

vilgotf left a comment

7596ff commented Sep 30, 2021

7596ff left a comment

7596ff Sep 30, 2021

baptiste0928 Sep 30, 2021

7596ff Sep 30, 2021

7596ff Sep 30, 2021

baptiste0928 Sep 30, 2021

7596ff Sep 30, 2021

7596ff left a comment

7596ff commented Oct 1, 2021

baptiste0928 commented Oct 1, 2021

7596ff commented Oct 1, 2021

vilgotf left a comment

vilgotf left a comment

baptiste0928 commented Oct 2, 2021

vilgotf left a comment

7596ff left a comment

Add a builder for CallbackData #1146

Add a builder for CallbackData #1146

Conversation

baptiste0928 commented Sep 1, 2021

vilgotf left a comment

Choose a reason for hiding this comment

baptiste0928 commented Sep 1, 2021

vilgotf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilgotf left a comment

Choose a reason for hiding this comment

7596ff commented Sep 30, 2021

7596ff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7596ff left a comment

Choose a reason for hiding this comment

7596ff commented Oct 1, 2021

baptiste0928 commented Oct 1, 2021

7596ff commented Oct 1, 2021

vilgotf left a comment

Choose a reason for hiding this comment

vilgotf left a comment

Choose a reason for hiding this comment

baptiste0928 commented Oct 2, 2021

vilgotf left a comment

Choose a reason for hiding this comment

7596ff left a comment

Choose a reason for hiding this comment