RFC: Location for model builders

[7596ff]

Right now, we have a few different model builders in different places:

• crate embed-builder
  • EmbedBuilder
• crate model
  • AllowedMentionsBuilder
  • GuildChannelFieldsBuilder
  • RequestGuildMembersBuilder

There is (as of 2021-07-20) an open PR to add a Command builder to model. Another aspect of this problem is that our builders perform validation tasks for HTTP requests.

We need to decide what to do here to make things more consistent. From what I can see there are a few options.

• Move all builders to the util crate, with their own separate features
• Move the Embed builder to the model crate, which is consistent with the other builders
• Unify builders and validation in their own crate
  (This relates to Unify validation errors into more concise error types #999 because at the moment we need to update two different const values to update the embed validation)

Edit: we'll hold off on making any changes related to this until after the next major release, as we already have enough breaking changes in 0.6

[laralove143]

Moving related builders in other crates would require mentioning them in the docs where they’re can be used, otherwise people would have to manually check and remember what builders are available. Also the structs returned from Client methods implementing Future kind of work like builders

[zeylahellyer]

My take is that we should have builders that perform validation in the model crate. Creating validated models is important for avoiding 400 errors so it makes sense to make it first-class with the models themselves. However, builders that are purely for ergonomic purposes should probably go in another crate like util. I don't believe it warrants its own crate.

[7596ff]

I'm willing to go with this solution

[laralove143]

I'm willing to go with this solution

It'd still be nice to have the models' documentation say it if there's a way to build it easier in the Util crate. I do agree that possibly opinionated things (Things that don't directly follow the Discord API) should be in a separate crate though, like the cache and utils crates

[vilgotf]

Having builders doing validation when some http requests already do (see create_global_command & it's methods) forces code duplication (since they're in different crates) and double validation. Since the request are the lowest common denominator I think they should do the validation.

[7596ff]

I'm planning to redo validation in a way that exposes constants for use in other crates

[casey]

I didn't discover EmbedBuilder when creating Embeds. Ideally, Embed would have a builder associated function that would return an EmbedBuilder, which would make it easily discoverable.

My personal preference would be that, whatever the crate split, models with builders have an associated builder function that returns a new builder, for consistency and discoverability.

[7596ff]

We could gate that on a feature so that you can optionally pull in whatever crate the builder is in

[casey]

As long as the function showed up in the docs, for discoverability, then that would be fine.

[zeylahellyer]

We could gate that on a feature so that you can optionally pull in whatever crate the builder is in

We wouldn't be able to depend on embed-builder in model. What this is represented as is embed-builder depending on model but model also depending on embed-builder. When releasing a new major version model might need to depend on a new major version of embed-builder but it hasn't been released yet. When releasing embed-builder it depends on the new major version of model, which hasn't been released yet. Since neither are released yet, neither can be released. So what we could do instead is link to the embed builder crate in the documentation for Embed and its related types.

[zeylahellyer]

I would like to call for a week long discussion period on the post and path forward I'm posting here. If we find severe issues we can change it and postpone, but if this sounds fine as-is I would like to move forward with it after a week.

Vocabulary

Cosmetic builder: a builder that does not perform validation and is purely to simplify initialization of a type or make it cleaner to read
Validated builder: a builder that does perform validation and has an associated error type

Long term plan

Here's what I believe the general "least controversial" long term consensus is based on current information and opinions while also getting stuff done:

• HTTP's validation constants (such as maximum message files allowed) will be exposed as a twilight_http::request::validate module
• New and current builders that perform validation will be moved from whatever crate they are in and go to model and be associated with their types (except for the Embed Builder)
• Cosmetic builders will go in util or a new builder crate; this is TBD, but before committing to a builder crate we should try out having them in util. Links to these builders will be on every struct that has one.

Unanswered questions for another time (don't respond to these):

• What should be done with the embed-builder crate? Deprecate it and move the builder to twilight_model::channel::embed or keep it as is?
• Should we create a new builder crate for cosmetic builders?

Short term plan

In the short term what we should do is:

1. link to the embed builder crate in the documentation for Embed and its related types
2. design an API for builders in util
3. move cosmetic builders to util and link to them from their model types
4. update and undraft PRs Add CommandBuilder #1048, Add a builder for CallbackData #1146 that are blocking on this issue

With regards to 2 (designing a util builder API), the API I'm thinking of is something like:

#[cfg(feature = "builder")]
pub mod builder {
    // Type being built is named `SomeCosmeticType`, so the module is named
    // accordingly.
    mod some_cosmetic_type {
        /// Builder to construct a [`SomeCosmeticType`].
        ///
        /// Because it's cosmetic and doesn't perform validation we don't need
        /// an associated error type.
        pub struct SomeCosmeticTypeBuilder;
    }

    pub use self::some_cosmetic_type::SomeCosmeticTypeBuilder;
}

One week period

I would like one week of discussion to discuss this. If there's any major changes we can set this back, but if everyone is happy with this general plan I would like to create issues for all of these items and create a new GitHub project to track it.

[casey]

What are the downsides of keeping cosmetic builders with the models that they build? It would aid discovery, and keep things consistent.

[zeylahellyer]

We want the model crate to only have type definitions mapping to the Discord API itself and validated builders

[casey]

Sorry if I'm not being clear. I understand what you want (i.e. not putting these things in the model crate), but I don't understand why you want it (e.g. because it will increase compilation times, or some other reason).

[zeylahellyer]

We want the model crate to be a direct mapping of the Discord API and not much else. We want the model crate to contain models. We don't want it to contain methods for converting between various things, we don't want it to contain integrations with the HTTP and cache crate similar to how other libraries do it to implement "convenient" methods on them, we want the crate to just contain DTOs with no complex logic for working with them. To quote the crate's root-level documentation:

These are in a single crate for ease of use, a single point of definition, and a sort of versioning of the Discord API. Similar to how a database schema progresses in versions, the definition of the API also progresses in versions.

The types in this crate are reproducible: deserializing a payload into a type, serializing it, and then deserializing it again will work.

Defined are a number of modules defining types returned by or owned by resource categories. For example, gateway are types used to interact with and returned by the gateway API. guild contains types owned by the Guild resource category. These types may be directly returned by, built on top of, or extended by other crates.

The only reason we want validated builders in it is because constructing valid models is a requirement for interacting with Discord's API, and we should enable - or in the future possibly require - constructing valid models regardless of whether one uses our HTTP library or their own.

[casey]

That does seem like a reasonable concern, although personally I think that weighing that against the trade-off that ergonomic builders would be less discoverable in another crate, some users might prefer to have ergonomic builders directly in the model crate.

Unless the ergonomic builder would introduce dependencies on the HTTP or cache crates?

[zeylahellyer]

The "we" is the aggregate consensus of what to do. Personally, I and some others don't want any builders in model (I don't even want any that aren't for validation purposes). Some want all builders in it. Some want to get rid of the embed-builder crate, I'm hesitant. This decision is a compromise and we can explore what needs changing once we've followed through on this direction.

[zeylahellyer]

One week period

I would like one week of discussion to discuss this. If there's any major changes we can set this back, but if everyone is happy with this general plan I would like to create issues for all of these items and create a new GitHub project to track it.

It has been one week. We will adopt these first steps and decide how to move forward after them. A project has been created: https://github.com/twilight-rs/twilight/projects/10

[PyroTechniac]

could a Builder trait be created that's similar to the Default trait found in std? where it implements on everything with a Builder and has a builder method that creates said builder?