-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
refactor: change xID
to xId
#6036
refactor: change xID
to xId
#6036
Conversation
Co-authored-by: Noel <buechler.noel@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things I'd like to see clarified. Also if you're doing this for consistency with casing across the library, why not change URL -> Url?
@@ -98,20 +98,20 @@ class Client extends BaseClient { | |||
: null; | |||
|
|||
/** | |||
* All of the {@link User} objects that have been cached at any point, mapped by their IDs | |||
* All of the {@link User} objects that have been cached at any point, mapped by their ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this and all the other JSDoc descriptions be kept as-is?
@@ -273,7 +273,7 @@ class Client extends BaseClient { | |||
|
|||
/** | |||
* Obtains a webhook from Discord. | |||
* @param {Snowflake} id ID of the webhook | |||
* @param {Snowflake} id The webhook's id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this specific ID be upper case? (and all others that follow the same "The x's id" pattern)
Edit: As standalone words, we'd prefer them uppercase, while joined with other words (i.e. identifiers), we'd like them in PascalCase, e.g. |
Isn't ID an abbreviation? So, following camelCase, shouldn't it be capitalized ( Reference: Microsoft Abbreviation Guidelines The word |
I don't see where in the website says that it should be |
The guidelines seem to tell us not to use an abbreviation at all. The final sentence "If you must use abbreviations, use camel case for abbreviations that consist of more than two characters, even if this contradicts the standard abbreviation of the word." Admittedly this abbreviation is not more than two characters. But we can do what we want regardless. |
I know it's way too late but I'm just seeing this now and I figured I'd mention that it's more than just being the same as other discord bot libraries. What about people with databases? They've probably stored a few structures as userID or guildID by now. The toJSON method obviously used to use ID instead of Id, and people have definitely used that for storing or sending json. So having the keys change could cause some confusion. Changing all instances of a function like resolveID to resolveId is one thing (although quite inconsistent), but then there's other parts like the short message references and toJSON objects. Plus, even if no bot works untouched with discord.js v13, that doesn't mean you should make that a promise. |
The same as #6189 - your fault for not sanitizing values and instead relying on whatever discord.js gives you (which by the way, is extremely inefficient storage wise, and often does not map to the raw data it accepts in the constructor). If I were you and I stored data from Discord, I'd make sure to pass each relevant column/property that I'd need when reading it back, as opposed to passing a dynamic set of properties with a dynamic set of values, since that gives a lot more control and makes migrations across Discord and Discord.js versions a lot easier when they somehow change or get removed. |
Perhaps I didn't do a great job at explaining the problem I see with this change. I definitely don't just throw random objects into my database, although I'm not sure if I can speak for everyone. |
If you don't just throw random objects into your database, then you're perhaps sanitizing it to some point. Here, the solution is very simple, map Anyways, if you want to continue any further, please ask in the official Discord server. |
Please describe the changes this PR makes and why it should be merged:
Did somebody order a pizza? Well, sorry to break it for you, it's another dish of breaking changes.
resolveID
.Barely any bot works untouched when switching from v12 to v13
and Sapphire is still unaffected, even by this.Basically, the gist of this PR is that all instances of
ID
have been changed toId
, that includesresolveID
->resolveId
,messageID
->messageId
,channelID
->channelId
,guildID
->guildId
, etc. And this has been in our backlog for around 2 years, so this change was coming out sooner or later.Status and versioning classification: