RFC: Model field getters #1407
PyroTechniac
started this conversation in
Development & RFCs
Replies: 1 comment
-
i don't see added value in hiding all fields for several reasons:
to me this doesn't seem worth the hassle of breaking every use of the lib without offering any benefit |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Model field getters
Right now, models have all their fields exposed directly. Take
Role
for example:This has some complications, one, is that we commit naming the fields as is, and changing them would be a breaking change (ignoring the model crates rules on breaking changes). Users are also able to construct their own versions of the type as they want, which could cause some headaches when debugging later on. This also allows model fields to be mutable, something we shouldn't allow (as the models are just representations of data discord sends).
Cache
The cache also suffers from the same issue of named fields, however it doesn't have to worry about naming changes, as we control the conversion between model -> cached model. This doesn't solve everything however, as most of the models are cached directly, with only a select few having custom types.
Solution: Getters
The current proposed solution (and request in #1365) is to hide all model fields in getters, and return references to the data, so that
Role
looks less like that and more likeWhich solves all the problems mentioned above, however it creates some new ones.
What this breaks
Short answer: A lot.
Long answer: Still a lot.
The first and foremost is that this breaks every single field access for all bots that use this, as they will all have to be converted to methods. This also breaks some things internally, mainly caching, as the cache holds a clone of most of the data, and the custom types use conversion methods, so having a lot of users/members means cloning a lot of
String
s and everything else they carry.This also brings into question whether or not to truly return references, as changing return types to references means that if anything needs an owned value, then it needs to clone all the data passed (this could mean something not so impactful like a
String
, or something bigger, such as aVec
ofRole
s).Send vs Receive types
There is also the issue with all the model types that are meant to be sent to discord vs received, such as
CallbackData
andAutocomplete
, to name a few.A few proposed solutions is to make them all into builder types, but then that causes issues with where they go, and what to do about the current builders.
If they all go to the
twilight-util
crate, like the builder forCommand
s andCallbackData
, then that means that if anyone uses any of them for building, they would need to bring in an entire other crate for it.If they're all moved to the
twilight-model
crate, then that is a breaking change for thetwilight-util
crate, as it means removing thebuilder
feature and all the current builders associated with them.A proposed solution is to move the different types into two main modules,
send
andreceive
, then go from there.Beta Was this translation helpful? Give feedback.
All reactions