search

wowdev / WoWDBDefs

Client database definitions for World of Warcraft
Other
246 stars 95 forks source link

First automation attempt, fixes signedness issues and adds missing builds #44

Closed Marlamin closed 5 years ago

Marlamin commented 5 years ago

I built some scripts that will hopefully eventually be able to do automated merges of new versions as soon as they're imported on my site (and exes are dumpable). Whether or not this will push to master or creates a new pull request/branch per build is something I'm still thinking about and is definitely up for discussion!

This is the first run of the merger merging all the versions in this repo (now also automated) with master. Even though the amount of changes is pretty large (99% build additions to build list) I really would appreciate if another person looked this over.

The signedness changes here and there are intended, they are the result of us moving signedness to version definitions instead of column definitions. In said move all the columns became int whether or not they were signed and haven't been fixed due to nothing being re-merged (until now).

Note that due to missing/or incorrect (I'm looking at you 7.0.3) patterns in DBDefsDumper, this script will only automatically merge builds after 25902 to play it safe. When patterns in Dumper improve, it is trivial to lower this build limit.

Again, would appreciate another check (might I suggest the raw diff) before merging this into master!

barncastle commented 5 years ago

Nice work! A brief glance over and this appears to be fine, I think I only saw two new definitions the rest are just sign changes and missing builds.

The only suspect changes I saw were:

Marlamin commented 5 years ago

That's mildly suspect, yeah. Other version definitions of those files agree with the new signedness though so not sure if those are wrong. If they are, stuff is being dumped wrongly for other version definitions as well. Will need to double check with meta.

funjoker commented 5 years ago

36 i adress this

Marlamin commented 5 years ago

Is that exactly the same case with those specific fields? If so, I guess this might be up to the reading implementation to deal with as I'm not sure this is within the scope of the project (or at least right now).

Marlamin commented 5 years ago

f075e7f is also fully automated! Haven't scheduled it yet to run after new builds, but will do when everything has been tested a bit more.

Marlamin commented 5 years ago

What @funjoker mentioned does seem to be the case here, but I'm leaving that as a separate issue for now. Everyone think this is good enough to go into master?

EDIT: Got approval via other channels, merging!