twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
673 stars 132 forks source link

refactor(gateway-queue)!: rewrite crate #2228

Closed vilgotf closed 1 year ago

vilgotf commented 1 year ago

Superceds and is a full rewrite of the proposed rewrite in #2172. Primarily motivated by removing the use of a background task Shard when waiting to identify. By returning a concrete Future type (implementing Debug) it becomes much easier to store it inside of Shard. Of course, I did not stop there, and you will find the gateway-queue's documentation much improved in addition to code reduction and an (initial) integration test suit. The provided InMemoryQueue should be more flexible for advanced use cases than LargeBotQueue whilst offering more features without added complexity than LocalQueue.

Refer to #2172 for motivation on changing the identify interval from 6 seconds to 5 seconds.

Gelbpunkt commented 1 year ago

We should also consider adding helper methods for SessionStartLimit -> Settings behind a feature flag

vilgotf commented 1 year ago

We should also consider adding helper methods for SessionStartLimit -> Settings behind a feature flag

I think that the time saved from such a helper is negated by the time spent from reading about and enabling the feature flag. Plus I don't like taking on a (comparatively massive) dependency for just one helper. Also note that Settings is currently a private type so exposing it requires

  1. coming up with a proper name, InMemoryQueueSettings is the obvious choice, but it's a mouthful
  2. documenting it and adding another item for new users to memorize.

In conclusion, I don't think such a feature and helper pair carry their weight.

Gelbpunkt commented 1 year ago

We should also consider adding helper methods for SessionStartLimit -> Settings behind a feature flag

I think that the time saved from such a helper is negated by the time spent from reading about and enabling the feature flag. Plus I don't like taking on a (comparatively massive) dependency for just one helper. Also note that Settings is currently a private type so exposing it requires

1. coming up with a proper name, `InMemoryQueueSettings` is the obvious choice, but it's a mouthful

2. documenting it and adding another item for new users to memorize.

In conclusion, I don't think such a feature and helper pair carry their weight.

Ah yeah I tend to agree. Overlooked that it is private and doesn't serve any purpose in public APIs. I'm just not happy with the current API, users who want to use the API for this will have to convert the u64 to a Duration manually. It's a bit verbose for something that should be the recommended way for bots that want to future-proof themselves. Maybe a helper method on SessionStartLimit that returns reset_after as a Duration would be within scope? Either way we should at least include an example of calling the endpoint and using its return value to construct a queue.

vilgotf commented 1 year ago

Maybe a helper method on SessionStartLimit that returns reset_after as a Duration would be within scope? Either way we should at least include an example of calling the endpoint and using its return value to construct a queue.

twilight-model should obviously not eagerly parse reset_after as a Duration and twilight-gateway-queue should obviously take a Duration as its temporal unit. Adding a helper method on a struct for just one field is not very clean in my opinion (although that is the status quo as can be seen with DiscriminatorDisplay. The method name is not obvious, both reset_after_as_duration and reset_after_duration don't sit right for me.) and I would instead like to see a custom type for reset_after. Essentially:

#[derive(Clone, Copy)]
ResetAfter(pub u64);

impl ResetAfter {
    fn as_duration(self) -> Duration;
}

Of course, this is an unfortunate name but making the API generic, as there are other situations where such a helper would be usefull, would not exactly be pretty either.

Conversely, calling Duration::from_millis is not too bad (just one method call), twilight-model clearly documents reset_after as being milliseconds, and Duration is very discoverable and core in Rust. I do think an example is good enough and it doesn't introduce any awkward API.

Gelbpunkt commented 1 year ago

Maybe a helper method on SessionStartLimit that returns reset_after as a Duration would be within scope? Either way we should at least include an example of calling the endpoint and using its return value to construct a queue.

twilight-model should obviously not eagerly parse reset_after as a Duration and twilight-gateway-queue should obviously take a Duration as its temporal unit. Adding a helper method on a struct for just one field is not very clean in my opinion (although that is the status quo as can be seen with DiscriminatorDisplay. The method name is not obvious, both reset_after_as_duration and reset_after_duration don't sit right for me.) and I would instead like to see a custom type for reset_after. Essentially:

#[derive(Clone, Copy)]
ResetAfter(pub u64);

impl ResetAfter {
    fn as_duration(self) -> Duration;
}

Of course, this is an unfortunate name but making the API generic, as there are other situations where such a helper would be usefull, would not exactly be pretty either.

Conversely, calling Duration::from_millis is not too bad (just one method call), twilight-model clearly documents reset_after as being milliseconds, and Duration is very discoverable and core in Rust. I do think an example is good enough and it doesn't introduce any awkward API.

Let's keep the API as is and improve upon the example instead. We might want to link there from the SessionStartLimit or GetGateway docs.