Skip to content
This repository has been archived by the owner on Dec 21, 2018. It is now read-only.

Add DDT and IMA and rebuild, fix a mistake with Ixalan's set code for standard, update the howto, fix a bug (?) with compareRelease.js, make Ormendahl's CMC = 0 #486

Merged
merged 6 commits into from
Nov 9, 2017

Conversation

bakert
Copy link
Contributor

@bakert bakert commented Nov 9, 2017

This pull request should almost certainly not be merged, yet. More on that in a bit.

I started this journey wanting to add a little change to rip.js to make Ormendahl, Profane Prince's CMC be zero and not null. It's our oldest outstanding bug: PennyDreadfulMTG/Penny-Dreadful-Tools#138, #435.

-					if (otherCard.hasOwnProperty('cmc'))
+					if (otherCard.hasOwnProperty('cmc')) {
					card.cmc = otherCard.cmc;
+					} else {
+						// The other card has no mama cost so it's a land, set CMC to land mana cost (0).
+						card.cmc = 0;
+					}

But to do the necessary rebuilding I had to add missing sets DDT and IMA. I added these and that then touched basically every set ever either because I messed something up or because IMA has sets from all of history or because WotC just added support for Simplified Chinese or ... something. I'm not sure.

I updated howto.txt as I went because it seemed out of date. I put that in a separate commit to the big one so it can be more easily reviewed.

While I was in there doing all this I changed the standard legal codes to 'XLN' instead of 'IXN' which I suppose is part of the cause of #479. This change is a (tiny) separate commit from all the rebuilding. I also regenerated (among many others) SOI and EMN which should shake loose the other part of #479.

I also found and logged #482, #483 and #485 as I was going through the process. I ended up re-entering the process manually about ten times. Maybe more. The number of sets with "tainted fields" started reducing at some point and I persevered. We should maybe have the machine do this automatically.

I ignored a bunch of output like this:

ARTIST: Day (26691) artist does not match MagicCardsInfo (/ap/en/131b.html).
"AnthonyChristopher S. WatersMoeller"

ARTIST: Illusion (27164) artist does not match MagicCardsInfo (/ap/en/129a.html).
"JohnDavid AvonMartin"

Mercadian Masques

FLAVOR: Aerial Caravan (19667) flavor does not match MagicCardsInfo (/mm/en/58.html).
"Successful delivery is *not* guaranteed."

I don't know how bad that is or if it's normal/not a big deal.

I ended up with a lot of output to do with basic lands and Simplified Chinese. I don't know if this is because I did something wrong or because something changed in Gatherer or somewhere else. Or just because IMA and/or DDT contain basic lands!

I had to change compareRelease.js because it wanted to look in web/json/*.json and my output was in json/ in the root. That change is in a separate commit for easier review. I wonder if this is to do with the symlinks mentioned in the README.

I didn't entirely complete the run of compareRelease.js because of #484 but my /tmp/changeSets.json had a looooot of set codes in it so I proceeded to pul request. Would love to understand that better.

The one reason I know why this almost certainly shouldn't be merged is because for some reason all rulings for all cards now seem to have the date 2017-11-17 (the release date of IMA?). Any advice on why that might be would be gratefully received.

I also committed package-lock.json to source control as recommended by npm. I'm pretty sure that was dumb because you'd have it in there if you wanted it in there I guess. But it's also not in a .gitignore so I don't know. Given #485 it's probably a particularly bad package-lock to commit.

Please let me know anything obvious that I've done wrong and I'll fix. It will be good to have the new sets and fix Standard legality and, of course, set Ormendahl's CMC to 0!

Or feel free to use my set definitions and code/howto changes and make a working set of changes!

Copy link
Contributor

@gwax gwax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the docs as you went!

Overall, your code changes seem 👍 but I'm a little bit concerned about some of the language and card order changes in some of your updated data files. I am guessing this is probably because of bugs elsewhere in the code.

Net positive, so let's get it in!

card.cmc = otherCard.cmc;
} else {
// The other card has no mama cost so it's a land, set CMC to land mana cost (0).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mama/mana/

@@ -45413,26 +45454,6 @@
"language": "Russian",
"name": "Гора",
"multiverseid": 149546
},
{
"language": "Spanish",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any idea why we might be losing the Spanish language translations here?

@@ -11576,8 +11596,8 @@
"name": "Isla"
}
],
"id": "0f1d449fe045409f0d2e7d9e8dcc2ac1ecd00c1a",
"imageName": "island1",
"id": "fa36af58b0ecb651a7e161537bac2f9daafe93b0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These card order changes are a little concerning to me, but I suspect that's a bug somewhere else.

@@ -156,37 +156,37 @@
"foreignNames": [
{
"language": "Chinese Simplified",
"name": "Bushi Tenderfoot",
"name": "无情的谦造",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's picking up the translations for the other half of the card.

@@ -2744,37 +2744,37 @@
"foreignNames": [
{
"language": "Chinese Simplified",
"name": "贤狐秋尾",
"name": "Kitsune Mystic",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a regression.

@gwax gwax merged commit 7e69e2a into mtgjson:master Nov 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants