Skip to content
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

Menagerie changes #304

Merged
merged 15 commits into from
Mar 24, 2020
Merged

Menagerie changes #304

merged 15 commits into from
Mar 24, 2020

Conversation

wvoigt
Copy link
Collaborator

@wvoigt wvoigt commented Mar 21, 2020

Starting the Pull for menagerie so we have a place to discuss as items get added.
Reference Issue #273

@wvoigt
Copy link
Collaborator Author

wvoigt commented Mar 21, 2020

I'm seeing the conflict errors. It has been a while since I did this and I know things have changed. I'm assuming I need to change the files in card_db_src and not src/domdiv/card_db. Is that correct? Or do I have it backwards? And do I need to still add on the new language entries in English to all the other languages there? Or is that done automatically now?

@wvoigt
Copy link
Collaborator Author

wvoigt commented Mar 21, 2020

FYI, to make this easier in the future (will there be any more expansions?), I'm entering the data in a spreadsheet (which is a lot easier than editing straight json.) Then I will write a short script that takes a csv file and makes json code that can be added to cards_db.json and en_us/cards_en_us.json files. I'll put them in the tools area when done in case anyone needs them in the future.

@sumpfork
Copy link
Owner

sumpfork commented Mar 21, 2020

Changes should only go into the top level db src dir, and ‘doit update_languages’ should run a modified version of your script to generate the version in the package. It’s still mostly similar to what you did, other than that it substitutes the English version wherever anything is missing, rather than requiring special markup as to what is untranslated.

If you don’t run the script yourself, CI should run it for you.

wvoigt and others added 4 commits March 22, 2020 14:28
Tools to help generate json code from a spreadsheet (.csv) for both the cards_db.json and en_us/cards_en_us.json files.
@wvoigt
Copy link
Collaborator Author

wvoigt commented Mar 22, 2020

I could use some help proof reading. I think I have most of this. But a second set of eyes would be good.
menagerie1.pdf All cards, no grouping
menagerie2.pdf Events and Ways grouped (within Menagerie)
menagerie3.pdf Events and Ways grouped globally in Extras.

@sumpfork
Copy link
Owner

Thanks, I'll try to get to this, and I apologize, there's something wrong with the travis check-in setup I tried to do to auto-compile the card db. I'll have to test it some more, but it seems to be getting confused about which branch to check out/commit to.

Copy link
Owner

@sumpfork sumpfork left a comment

Choose a reason for hiding this comment

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

Looks like the tests need to be updated with the new card number?

I also have the concern that the non-JSON files with get out of date with the JSON files and be hard to reconcile. Are you thinking the CSVs should become source of truth? Otherwise there should probably be checks to make sure the scripts aren't duplicating cards, etc.

src/domdiv/tools/new_cards.py Outdated Show resolved Hide resolved
@sumpfork
Copy link
Owner

https://travis-ci.org/github/sumpfork/dominiontabs/builds/666010019 - please install pre-commit. There's a note as to what to do in the README.

@sumpfork
Copy link
Owner

And sorry travis isn't updating the PR statuses - not sure why, trying to fix.

@sumpfork sumpfork merged commit 5fcfcdb into master Mar 24, 2020
@sumpfork sumpfork deleted the menagerie branch March 24, 2020 05:29
mrgrain pushed a commit to mrgrain/dominiontabs that referenced this pull request May 6, 2021
* New menagerie images
* Added options to deal with the new Way cards
* Added Menagerie set and new card types
* Tool to help bring in a large number of new cards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants