-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
feat!: rework model enums #2045
Draft
zeylahellyer
wants to merge
13
commits into
next
Choose a base branch
from
zeyla/feat-model-rework-enums
base: next
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeylahellyer
added
t-feature
Addition of a new feature
c-http
Affects the http crate
c-cache
Affects the cache crate
c-model
Affects the model crate
c-gateway
Affects the gateway crate
m-breaking change
Breaks the public API.
c-standby
Affects the standby crate
c-all
Affects all crates or the project as a whole
c-util
Affects the util crate
t-refactor
Refactors APIs or code.
w-do-not-merge
PR is blocked or deferred
w-unapproved
Proposal for change has *not* been approved by @twilight-rs/core.
w-needs-testing
PR needs to be tested.
c-validate
Affects the validate crate
w-needs-more-docs
Needs more documentation before being worked on.
t-perf
Improves performace
c-book
Affects the book
labels
Jan 8, 2023
This comment was marked as resolved.
This comment was marked as resolved.
zeylahellyer
added a commit
that referenced
this pull request
Jan 15, 2023
Adds a typed Locale struct with associated constants for all known values, replacing the Strings used in the codebase. The Locale type has two methods of note: `english_name` and `native_name`. These values are taken from the Discord documentation: <https://discord.com/developers/docs/reference#locales> This PR is meant to show how the new raw value representation introduced in PR #2045 looks when "typing" a value. This was previously left to the user to know what localizations might exist and to ensure a typo isn't made. This `Locale` type is still functionally a string in all but name (literally, as it implements the same traits and derefs in the same way String does), but it contains methods and the known values registered by Twilight as "sugar" on top of it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
c-all
Affects all crates or the project as a whole
c-book
Affects the book
c-cache
Affects the cache crate
c-gateway
Affects the gateway crate
c-http
Affects the http crate
c-model
Affects the model crate
c-standby
Affects the standby crate
c-util
Affects the util crate
c-validate
Affects the validate crate
m-breaking change
Breaks the public API.
t-feature
Addition of a new feature
t-perf
Improves performace
t-refactor
Refactors APIs or code.
w-do-not-merge
PR is blocked or deferred
w-needs-more-docs
Needs more documentation before being worked on.
w-needs-testing
PR needs to be tested.
w-unapproved
Proposal for change has *not* been approved by @twilight-rs/core.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description is a work in progress
Rework model enums to more cleanly map to raw values and make it so that variants unknown to the library don't feel like "second class" citizens to be avoided.
This PR is possibly aimed for Twilight 0.16, but perhaps more likely 0.17.
Motivation
I was reviewing and merging PRs and saw PR #1922, which is a PR that changes the
Guild::afk_timeout
type from an integer to an enum. A review comment suggested using associated constants for AFK timeout values instead of enum variants. I tended to agree, since AFK timeout values are somewhat "arbitrary", in that rather than being incremental values (1, 2, 3, ...) the accepted values were a range of indeterminate values (60, or "one minute"; 300, or "five minutes"; 900, or "fiften minutes"; etc.). Reviewers on the PR agreed this direction looked cleaner and nicer to use. It looked like this:`AfkTimeout` implementation
Later, I created PR #2018, which added a module with public constants (not on a type) of known valid scopes:
It was again suggested to use a type with associated constants since it felt like a good design in the AFK timeout PR. And, again, I agreed. However, doing so for scopes wasn't as clean: scopes are strings in the underlying type, while the AFK timeout struct uses an integer. You need to have owned strings for deserializing from serde contexts, but borrowed (and static) values for associated constants.
Cow
s aren't a solution, because they don't work in all constant contexts. Thus, I madeScope
be a wrapper over some bytes so it could be used in associated constants and (de)serialized from/to serde contexts.Now that I had this working I saw that this was a lot of work just to make a
Scope
type. We have a lot of enums in thetwilight-model
crate that are similar toAfkTimeout
andScope
, in that they have "arbitrary values". This presented me with a question: what if we extend this to all enums like it? Other enums similar to these are composed of types such asAutoArchiveDuration
,GuildFeature
, andIntegrationExpireBehavior
.AutoArchiveDuration
is an enum with variants ofHour
,Day
,ThreeDays
,Week
, and anUnknown
variant for values that aren't registered with Twilight.GuildFeature
is an enum with variants for known features, such asAnimatedBanner
,AutoModeration
, andDiscoverable
, and again with anUnknown
variant.IntegrationExpireBehavior
is an enum that declares what should happen when an integration expires, and its variants areKick
,RemoveRole
, andUnknown
.These are three enums that feel right as being classified as "arbitrary values".
AutoArchiveDuration
isn't incremental and contains values that map more to what makes sense for Discord's UI;GuildFeature
is a growing list of strings that are entirely arbitrary, and is added to and removed from often in Discord's API; lastly,IntegrationExpireBehavior
is a type with variants where the mapped values can't be made sense of on their own: no one would guess that the listed order of variants above was in the wrong order, becauseKick
is actually value 1 whileRemoveRole
is actually value 0.The point can be made that this is true of every enum we have mapping to a value. For example,
ChannelType
has many variants that are mostly incremental, with value 0 beingGuildText
and value 10 beingAnnouncementThread
. These values, although nonsensical on their own, are important. They're added to as the API and Discord's types of channels grows. Twilight certainly will never have all of them at all times. The raw integers are the value, not the enum variant name.As a result of this thinking, this PR modifies every enum without data (such as
Component
) to use this scheme. It makes every type consistent; it makes one think about the value itself and not the abstraction of the value; it's clean to use; and "unknown values" no longer feel like a side-effect, they're a feature.Definition Examples
Explicit Content Filter (Integer)
Before
After
Guild Feature (String)
Before
After
Use Case Examples
Todo
PR Todo
todo!()
comments; not difficult, just didn't get to themknown_string
more betterknown_string
name
methods on remaining model types (these exist on some already, and was extended to all)name
) and trait implementations (such asDebug
,FromStr
)