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

Split GuildThread from GuildChannel #3026

Open
wants to merge 158 commits into
base: next
Choose a base branch
from

Conversation

GnomedDev
Copy link
Member

@GnomedDev GnomedDev commented Nov 12, 2024

This fixes #2991, via the solution voted for on the serenity discord.

@github-actions github-actions bot added model Related to the `model` module. builder Related to the `builder` module. cache Related to the `cache`-feature. http Related to the `http` module. utils Related to the `utils` module. gateway Related to the `gateway` module. examples Related to Serenity's examples. labels Nov 12, 2024
@GnomedDev GnomedDev force-pushed the thread-split branch 4 times, most recently from d87a2db to 037c2d6 Compare November 14, 2024 01:23
GnomedDev and others added 17 commits November 15, 2024 19:08
…erenity-rs#2646)

This avoids having to allocate to store fixed length (replaced with normal
array) or fixed capacity (replaced with `ArrayVec`) collections as vectors for
the purposes of putting them through the `Request` plumbing.

Slight behavioral change - before, setting `params` to `Some(vec![])`
would still append a question mark to the end of the url. Now, we check
if the params array `is_empty` instead of `is_some`, so the question
mark won't be appended if the params list is empty.

Co-authored-by: Michael Krasnitski <[email protected]>
These are unnecessary. Accepting `impl Into<Arc<T>>` allows passing either `T` or `Arc<T>`.
This trades a heap allocation for messages sent along with thread
creation for `Message`'s inline size dropping from 1176 bytes to 760
bytes,
…l models (serenity-rs#2656)

This shrinks type sizes by a lot; however, it makes the user experience slightly
different:

- `FixedString` must be converted to String with `.into()` or `.into_string()`
  before it can be pushed to, but dereferences to `&str` as is.
- `FixedArray` must be converted to `Vec` with `.into()` or `.into_vec()`
  before it can be pushed to, but dereferences to `&[T]` as is.

The crate of these types is currently a Git dependency, but this is fine for
the `next` branch. It needs some basic testing, which Serenity is perfect for,
before a release will be made to crates.io.
…enity-rs#2668)

This commit:

- switches from `u64` to `i64` in `CreateCommandOption::min_int_value` and
`CreateCommandOption::max_int_value` to accommodate negative integers in
Discord's integer range (between -2^53 and 2^53). Values outside this
range will cause Discord's API to return an error.
- switches from `i32` to `i64` in `CreateCommandOption::add_int_choice` and
`CreateCommandOption::add_int_choice_localized` to accommodate Discord's
complete integer range (between -2^53 and 2^53). Values outside this
range will cause Discord's API to return an error.
This cache was just duplicating information already present in `Guild::members`
and therefore should be removed.

This saves around 700 MBs for my bot (pre-`FixedString`).

This has to refactor `utils::content_safe` to always take a `Guild` instead
of`Cache`, but in practice it was mostly pulling from the guild cache anyway
and this means it is more likely to respect nicknames and other information,
while losing the ability to clean mentions from DMs, which do not matter.
`Embed::fields` previously had to stay as a `Vec` due to `CreateEmbed` wrapping
around it, but by implementing `Serialize` manually we can overwrite the
`Embed::fields` with a normal `Vec`, for a small performance hit on
serialization while saving some space for all stored `Embed`s.
Simply missed these when finding and replacing.
This uses the `bool_to_bitflags` macro to remove boolean (and optional boolean)
fields from structs and pack them into a bitflags invocation, so a struct with
many bools will only use one or two bytes, instead of a byte per bool as is.

This requires using getters and setters for the boolean fields, which changes
user experience and is hard to document, which is a significant downside, but
is such a nice change and will just become more and more efficient as time goes
on.
…rs#2681)

This swaps fields that store `Option<Int>` for `Option<NonMaxInt>` where the
maximum value would be ludicrous. Since `nonmax` uses `NonZero` internally,
this gives us niche optimisations, so model sizes can drop some more.

I have had to include a workaround for [serenity-rs#17] in `optional_string` by making my
own `TryFrom<u64>`, so that should be removable once that issue is fixed.

[serenity-rs#17]: LPGhatguy/nonmax#17
A couple of clippy bugs have been fixed and I have shrunk model
sizes enough to make `clippy::large_enum_variant` go away.
A discord bot library should not be using the tools reserved for low
level OS interaction/data structure libraries.
GnomedDev and others added 7 commits November 15, 2024 19:23
This deletes methods which simply pass through to their `id` fields, as
they are:
- Misleading, as a user may do an HTTP call or cache lookup to get a
model just to use it's ID
- Lead to a bunch of maintenance issues when having 2..=4 copies of
every method.
- Hurt compile times, as rustc is having to check signatures, calls,
construct async state machines, and maybe even inline to generate code
multiple times.

For methods which simply only used their `id` field but did not have a
version on their respective ID, they were moved to their ID.

This doesn't have a corresponding deprecation PR to current, as current
still has permission checks and other "helpers" which rely on the model.
This exposes the `bytes` dependency but in turn allows for people to
pass responses directly from reqwest to discord without cloning the
data, as well as allowing reuploading cached data without cloning or
leaking to get a static reference.

The bytes dependency also has to be made non-optional, but this is fine
as it was already due to `aformat` depending on it via `bytestring`.
This separates the the builders for creating versus editing a command,
since it's not possible to change the `type` of a command (in serenity
this is the `kind` field). Also, the `name` field is not required when
editing a command.
Improved wording and removed some sentences that are no longer relevant.
These docs are mostly the ones that were touched by serenity-rs#2988.
Discord documents this as always present, even with Ping interactions.
This follows up on serenity-rs#3037, and also removes the now unnecessary Option
around the GuildChannel param.
@GnomedDev GnomedDev force-pushed the thread-split branch 6 times, most recently from 16cc66c to f400ac4 Compare November 17, 2024 21:36
@GnomedDev GnomedDev marked this pull request as ready for review November 17, 2024 21:38
@GnomedDev
Copy link
Member Author

GnomedDev commented Nov 17, 2024

This is ready for review, the main changes are:

  • GuildChannel is now:

    • BaseGuildChannel (shared fields)
    • GuildChannel (channel-exclusive)
    • GuildThread (thread-exclusive)
    • GenericGuildChannelRef (an enum over references to both)
  • PartialChannel is now:

    • BaseInteractionChannel (shared fields)
    • InteractionChannel (channel-exclusive)
    • InteractionThread (thread-exclusive)
    • GenericInteractionChannel (an enum over the both)
  • ChannelId is now:

    • GenericChannelId (shared purpose/methods)
    • ChannelId (channel-exclusive)
    • ThreadId (thread-exclusive)

The IDs have methods to convert between, such as:

  • GenericChannelId::expect_channel and GenericChannelId::expect_thread which casts the ID types without touching the value
  • GenericChannelId::split which calls both and returns both for when something needs to handle both cases
  • ThreadId::widen and Channelid::widen which cast to GenericChannelId for when you have a specific ID and need to "widen" the number of methods you can call.

GenericGuildChannelRef is returned from the new Guild::channel method, which looks the GenericChannelId up in both Guild::channels and Guild::threads.

@arqunis arqunis added enhancement An improvement to Serenity. breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users labels Nov 17, 2024
Copy link
Member

@jamesbt365 jamesbt365 left a comment

Choose a reason for hiding this comment

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

This looks really good, not 100% sure if I like the name changes for PartialChannel, but I can't think of anything better right now so I think its passable.

@peanutbother
Copy link

I also think the naming could be improved since we are explicitly talking about channels and threads.
My proposal in discord was the following:

How about

enum MessageOrigin {
    Channel(ChannelId),
    Thread(ThreadId)
}

seems a better naming to me since I see Threads as their own thing so I would not name a message's origin a channel

In that naming scheme I would name Base* rather Origin* and in some contexts append or prepend Message* or Interaction* accordingly.

@GnomedDev
Copy link
Member Author

@peanutbother I don't fully understand how this "origin" naming improves things? From how I processed that I think you are saying OriginGuildChannel which doesn't make too much sense, and we already have prepended Interaction where possible (as the other types are truly generically used everywhere).

@peanutbother
Copy link

peanutbother commented Nov 19, 2024

I mean something like MessageOrigin and InteractionOrigin as base names instead of the chosen base names as for e.g. threads and channels the base name is still channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users builder Related to the `builder` module. cache Related to the `cache`-feature. enhancement An improvement to Serenity. examples Related to Serenity's examples. gateway Related to the `gateway` module. http Related to the `http` module. model Related to the `model` module. utils Related to the `utils` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.