-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988
base: master
Are you sure you want to change the base?
Conversation
… its core counterparts * all fields will now follow the UpperCamelCase naming standard * fields that reference a DB2 storage will use the DB2 storage name followed with the ending 'Id' (e.g. CreatureTypeId, FactionTemplateId) * changed some field types to match their query packet data counterpart
69ccc7b
to
287ce60
Compare
CHANGE COLUMN `faction` `FactionTemplateID` SMALLINT UNSIGNED NOT NULL DEFAULT 0 AFTER `VignetteID`, | ||
CHANGE COLUMN `npcflag` `NpcFlags` BIGINT UNSIGNED NOT NULL DEFAULT 0 AFTER `FactionTemplateID`, |
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.
May I ask why ID
would be capitalized in the column name but not Npc
?
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.
ID was originally intended to be Id which was changed later because Blizzard uses ID for their packet and db2 fields and to stay in line with the hotfixDB, I resorted to ID.
@@ -0,0 +1,26 @@ | |||
ALTER TABLE `creature_template` | |||
CHANGE COLUMN `entry` `CreatureID` INT UNSIGNED NOT NULL DEFAULT 0 FIRST, |
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.
Should this be just ID ? What do other tables use as PK name ?
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.
entry, however, since we use CreatureID in all other cases (Creature text, sparring, gossips) we should just turn it consistent
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.
id / entry is far more common as a convention. It also makes much more sense, we already know what table it belongs to.
@trickerer @Rochet2 |
I love the motivation behind this PR. The inconsistent naming is something I have moaned about for a long time, so good on you for taking this work on. However I strongly believe we should not be handling it like this for 3.3.5. Even for master, we should not be modelling the server DB as a mirror of the client DB. This is going to cause a huge amount of frustration and pain. Adding creature as a prefix to almost every column is not helpful. It's in the creature class/domain, we know exactly what data it relates to. It's just more characters to type each time you want to perform any query, and when joining tables it's extra redundant information to include. When we join the table, we give it a name -- so now it's going to be Also bear in mind case sensitivity. Depending on the OS, it does matter. Now we have to write exact case sensitivity rather than knowing everything is lowercase. Again, I think this complicates things without providing any benefit. Blizzard uses PostgreSQL which does not have the same case sensitivity issues as MySQL. This is creating a lot of breaking changes. The refactor would be far less impactful if we stuck to the existing naming conventions, i.e: id/entry as the primary key (pick one or the other) and just naming the fields without considering which other client DB it might point to. I have many tools that I have written over the years that are all going to be broken by this change. The uptake is straight-forward but there's no need for it to be so intrusive. The documentation for the tables is actually quite good. I use it as a reference frequently. It does not make the columns any more clear by making them super verbose. We have full control to compose and structure the data how we like on the backend. Let's not fall into the trap of copying the DBFilesClient which are only a limited subset that has been transformed from Blizzard's schema. Master has DB2's and an entirely different schema. As easy as it would make a maintainers life to have the same in both versions, it does not actually make sense. They are completely different. TL;DR: please reconsider how we approach this, at least for 3.3.5. edit: There's only a few columns prefixed with creature. My primary problem is with the primary key, it should remain id or entry. The other columns are less contentious, but I still believe we should be avoiding mirroring the client DB. |
ALTER TABLE `creature_template` | ||
CHANGE COLUMN `entry` `CreatureID` INT UNSIGNED NOT NULL DEFAULT 0 FIRST, | ||
CHANGE COLUMN `name` `Name` MEDIUMTEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL AFTER `KillCredit2`, | ||
CHANGE COLUMN `femaleName` `NameAlt` MEDIUMTEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL AFTER `Name`, |
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.
Why do we pick a short string here and have verbose names everywhere else?
name2 or nameAlternative. Maybe name1 and name2? That would actually make querying easier since name
is a reserved word in MySQL, no need to escape anymore.
@stoneharry a couple of notes unrelated to the pr: columns are case insensitive in mysql, and joins with tables should always use aliases, like JOIN creature_equip_template AS cet |
SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.CreatureID = equip.CreatureID vs SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.id = equip.id I think the latter is much better. |
That should be SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.ID = equip.CreatureID I actually prefer this: SELECT ... FROM creature_template AS c
JOIN creature_equip_template AS cet ON c.ID = cet.CreatureID |
My point was if we are standardising the column names, we should be using id not CreatureID. What you call your tables when joining is irrelevant. |
Maybe a simpler example is: SELECT * FROM creature_template WHERE CreatureID = 10 We already know we are targeting the creature_template table, it should just be ID. If we really want to be pedantic, maybe this conflicts with SELECT * FROM creature WHERE CreatureID = 1;
SELECT * FROM creature_template WHERE CreatureTemplateId = 10; That would be even more horrendous. |
CreatureID in creature_template table as PK is just wrong, PKs should not have the table name in them. SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.ID = equip.CreatureID creature has ID, foreign table creature_equip_template has CreatureID (should be CreatureTemplateID even) |
This PR proposes renaming the PK to CreatureID, which is what my samples are trying to show the problems with. Likewise, creature_equip_template should just be |
Some of the oldest tables in the database did not age well and have many inconsistent naming styles. This PR will get us one step closer to escaping this mess.
Changes proposed:
Tests performed: