-
-
Notifications
You must be signed in to change notification settings - Fork 66
Lavish livestock address review comments #1244
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
Lavish livestock address review comments #1244
Conversation
|
I don't think removing the |
|
I actually firmly disagree. I think the CSV was actually more buried and less friendly since it was in the |
|
I can change it back to the CSV if that's desired, but I do disagree. Not enough to die on this hill and stall this PR longer than needed though 😅 |
|
To be fair, we do have a lot of single lists within code and not separated. I don't know if we have a single column CSV anywhere and it does look weird to me, to be in a CSV rather than a format that accepts a simple list (of like 6 or 7 entries too, which is not much) The |
gm4_lavish_livestock/data/gm4_lavish_livestock/guidebook/lavish_livestock.json
Outdated
Show resolved
Hide resolved
gm4_lavish_livestock/data/gm4_lavish_livestock/templates/advancement/breed.json
Outdated
Show resolved
Hide resolved
misode
left a comment
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.
Much better, but I'm not a fan that this module uses both bolt and jinja templates. We should pick one of the two for each module (and in the future ideally only one for the whole repo imo), but that doesn't necessarily need to be in this PR
|
Alright renamed the list to |
* Uninitialized Value * Remove unneeded score reset * Rename advancement criteria * CSV -> List in generate.py * Basic Guidebook * Change guidebook list for translated entity names * beeps is this good? * Move list to beet.yaml * Rename list to `livestock`
generate.py