Remove redundant builders and add Discord docs

[kangalio]

Some of the builders were representations of the exact same Discord types as model types, so I deduplicated them

And I added Discord docs links to the builders and some model types

[mkrasnitski]

I think deduplicating using type aliases is problematic, because it pollutes the original model types with methods that are only useful when building a request. From my point of view, those methods make no sense to call on a model type that was returned from the API. Also, clippy complains about #[must_use] a lot, and you can't mark a type alias as such, so it would require decorating the original model type with #[must_use] instead.

[kangalio]

From my point of view, those methods make no sense to call on a model type that was returned from the API

Can you explain why you feel that way? At their core, they are just convenience methods to edit fields, which is not a builder-specific concept

[kangalio]

What I mainly find interesting about this PR is how it shows that we don't really need the noisy methods on builders anymore (independent of deduplicating them with model types; if would work with the duplicated builders just as well):

- .title("This is a title")
+ title: Some("This is a title".into()),
- .description("This is a description")
+ description: Some("This is a description".into()),
- .image("attachment://ferris_eyes.png")
+ image: Some(CreateEmbedImage::new("attachment://ferris_eyes.png")),
- .fields(vec![
+ fields: vec![
-     ("This is the first field", "This is a field body", true),
+     EmbedField::new("This is the first field", "This is a field body", true),
-     ("This is the second field", "Both fields are inline", true),
+     EmbedField::new("This is the second field", "Both fields are inline", true),
+     EmbedField::new("This is the third field", "This is not an inline field", false),
- ])
+ ],
- .field("This is the third field", "This is not an inline field", false)
- .footer(footer)
+ footer: Some(footer),

https://github.com/kangalioo/serenity/commit/d213b69f05ea48b318db99d4cdbfced60d85ded7

I'm thinking what we could do is, instead of gating the entire builder structs behind the builder feature, we gate just their impls. The impls are a lot of code, so this can save compile time. AND: because the builders themselves are no longer gated, we can now take builders in Http directly (fixing point 1 of #1905)

[mkrasnitski]

From my point of view, those methods make no sense to call on a model type that was returned from the API

Can you explain why you feel that way? At their core, they are just convenience methods to edit fields, which is not a builder-specific concept

My main reason for feeling this way, is that I feel any data that's returned from the API should be immutable. Take embeds, for example. If you have an Embed that was returned from the API, and you change some field values, those effects won't be reflected until you use it as a builder with EditMessage. So, the representation of that message (through the model type) will be incorrect until the request is made. Having the model types serve this dual purpose I think allows for introductions of invalid states, especially if you forget to actually submit the builder. Adding #[must_use] on the type would address that specific problem, (as it did with the builder types), but I feel overall this is a conflation of two separate concepts.

[mkrasnitski]

I think I agree with the idea of moving the builder feature gate, to mirror how the model feature gate is used presently. The builder methods are useful if you don't want to put a bunch of field: None in all your struct declarations, and having to explicitly call .into on the fields (or otherwise explicitly construct the type like EmbedField::new(...)) is a bit noisy. They're mainly a convenience, except for Builder::new, which enforces required fields, whereas constructing the builder type directly doesn't do any enforcement.

[kangalio]

those effects won't be reflected until you use it as a builder

Ah I see, thanks for explaining your mental model. I'll change this PR to use newtypes instead of type aliases

[mkrasnitski]

Scratch what I said in my earlier comment; since builder fields are private, there isn't really any other way to instantiate them than with the builder methods. Therefore, exposing the structs themselves but not their methods wouldn't actually serve any value, at least not at the present moment.

[kangalio]

Scratch what I said in my earlier comment; since builder fields are private, there isn't really any other way to instantiate them than with the builder methods. Therefore, exposing the structs themselves but not their methods wouldn't actually serve any value, at least not at the present moment.

Yeah, the endeavor would entail making the fields public, as seen in one of the commits of my demo branch linked above: https://github.com/kangalioo/serenity/commit/a1b63f16d0415411c9a681549ed7a2f2f6e6fbdf

[mkrasnitski]

I think doing so would adversely affect the field enforcement we already do, for little or no overall benefit to most users. I think a little bit of codegen is worth the price for the type safety the builder methods provide. Also, I recommend you actually try just moving the feature gate and nothing else, and compare the output of cargo build --timings. Or, maybe just build without the builder feature (I doubt the struct definitions themselves add much codegen at all, but maybe the serde derives are not insignificant).

[kangalio]

You're right that idea would need much more consideration and experimentation

---

Independent from that; are there other doubts or blockers about this PR?

[kangalio]

I can't do this

I tried to rebase this fucking PR three times

Every time git fucked up. Duplicating code; trying to smuggle outdated code in; removing important code; breaking all hell loose

The only option is to recreate this PR from scratch