lionel-panhaleux / krcg

VTES python library providing a variety of tools. It uses the official VEKN cards list, TWDA and rules.
MIT License
10 stars 9 forks source link

Label rulings with the card id #17

Closed GiottoVerducci closed 4 years ago

GiottoVerducci commented 4 years ago

In the rulings.yam, instead of having entries with the card name only, prepend the card id;

For instance

100003:Aaron's Feeding Razor:

Keeping the name is useful when reading the source file. However, when processing the file, it should be discarded and the rendering of the ruling should be done by retrieving the card name by its id.

lionel-panhaleux commented 4 years ago

I was unsure about using the IDs as different sources / softwares use different IDs for cards (ie. Ke told me Amaranth has its own internal IDs: for him to use something like KRCG he needs to fetch cards by name). So, as card names are unique by design, they're used as identifiers throughout KRCG.

Switching to card ID makes sense, especially for renames (like the recent Rego Motus → Rego Motum), though in this case and if we use IDs, names in the rulings file may continue to use old names, which is not ideal for consistency.

I'm OK with switching to IDs, though I think it would be better to modify vtes.py so that the dict uses IDs too. What do you think ?

Note that a mistyped card name in rulings.yaml will break the tests, so we need not to worry about misspellings in the file.

GiottoVerducci commented 4 years ago

The id in the official reference files are guaranteed to be unique and never change. About the ruling file: that is the reason why I said: "Keeping the name is useful when reading the source file. However, when processing the file, it should be discarded and the rendering of the ruling should be done by retrieving the card name by its id." so that on the page the name is always correct. I haven't looked at all the implementations, but yes, it would be probably better to have the dictionary indexed by the id;

GiottoVerducci commented 4 years ago

A better syntax would probably be id:name so that multiple cards (on the general rulings for instance) can be separated with slashes.

Eg. 100771:Form of Mist / 101222:Mirror Image (commas can't be used to separate cards as they are used in card names).

lionel-panhaleux commented 4 years ago

Well, columns are used too (powerbases, traditions). We can opt for pipe |or "/" I guess, if we're confident it'll never make its way in a card name.

Too bad PyYaml will not accept lists as keys 😕 That would have be ideal.

For now I see too solutions:

  1. Rely on the tests to guarantee card names stay up-to-date. Drawback: a release is need when a card name changes and a test should check that the app continues to work when there is a mismatch in names.
  2. Use IDs in the keys. Drawbacks: PyYaml does not support lists as keys, so a separator must be used. | will do for now, but another solution (or char) would be needed if it is ever used in a card name. Card names should still be checked for consistency by the tests.

I'd say 2. looks better to me. If it's alright with you, I'll propose a PR for it. Any preference between | and / ? If we use a third files for general rulings (see https://github.com/lionel-panhaleux/krcg/issues/18) we won't need multiple-cards keys.

GiottoVerducci commented 4 years ago

2 is fine. There won't be any pipe in card names.