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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
More strict data type for member roles #2097
Comments
Ah, I didn't realize there are more fields with the same pattern To be clear, the root cause for what I'm trying to raise is that a duplicated values will result in an error when the Discord's API gets called. If the API can tolerate duplicates, it might be fine to just keep it with I obviously don't have the data on how people uses the library, but for my usecase (web devs) generally the performance hit is negligible compared to time spent waiting on I/O. Not to mention that this fields usually only gets processed once in a while. If I can raise a point, I'd say that tl;dr: it might be more proper to change the fields that better correspond to what Discord APIs expects Having said that, I understand if this changes might not be considered, since it will break existing Serenity APIs since it's a publicly accessible fields |
Any others' opinions? Use |
Just that from me 馃憤 |
Hello 馃憢
I've been debugging an issue, it seems to be related to how roles are handled.
Based on the message, I'd assume that there's at least a requirement that when we add a role to a server member, the list of roles shouldn't be duplicated. If this is the case, we could change the
roles
field data type into aHashSet
This ensures that the roles can never be duplicated. Also to note that this would be an API break since it's a public item
I've taken a peek at other (internal implementation) location, and I think changing the type shouldn't be very difficult, since the operation on
Vec
should be able to applied also toHashSet
As a context: the way I handle the roles assignment also uses
HashSet
, and currently I just convert it toVec
specifically for this fieldThe text was updated successfully, but these errors were encountered: