-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Used the COMPOUND_STRING macro to unify item descriptions and item data #3432
Conversation
Misc. Changes: -Corrected the Serious Mint's description. -Moved the X_ITEM_STAGES and CONFUSE_BERRY_HEAL_FRACTION macros to the top of the file for consistency's sake. -Corrected the Tapunium Z's description.
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.
I love this change.
I think I like your formatting better than the other obvious alternative:
.description = COMPOUND_STRING(
"A tool used for\n"
"catching wild\n"
"Pokémon."
),
But I suppose reasonable people could disagree about whether it's better to have 5 lines with little indentation, or 3 lines with a lot of indentation. I'll let people fight it out in the comments to this PR 🤣
This is a great PR Lunos. Formatting is better too. Is there something missing since you set it as a draft? |
Both PRs you were waiting for have been merged in 👀 |
Hell yeah! Gonna pull and solve the merge conflicts then. |
…expansion into itemDescriptions
5e41b02
to
a2bad2e
Compare
Alright, after some crazy developments on my end and Git moving me to a non-branch for reasons that I don't understand, this PR is ready for review. I pulled upcoming, solved the conflicts and moved the definitions of The definitions of |
c37cd89
to
a7d2a2c
Compare
And now I had to move I really don't like to have item data related constants split across 2 files like that, but I cba to think of a good solution to address that so I'll leave that to whoever else may care enough. It's not in-scope for this PR regardless. |
Like what we do for moves and Pokédex entries that share a description, I think we should have some static declarations at the top of the file for shared descriptions (I'm thinking e.g. Poké Toy and Fluffy Tail) to avoid duplicate strings (that modern doesn't consolidate, as we have found out). |
I don't like the inconsistency that creates at the time of editing the strings, and it's not like item description strings aren't duplicated even in vanilla, but I'll look into it. |
Ready for re-review. |
…ta (rh-hideout#3432) * Used the COMPOUND_STRING macro to unify item descriptions and item data Misc. Changes: -Corrected the Serious Mint's description. -Moved the X_ITEM_STAGES and CONFUSE_BERRY_HEAL_FRACTION macros to the top of the file for consistency's sake. -Corrected the Tapunium Z's description. * Oops. I forgot to delete the old .description of the X Attack * Removed definition of CONFUSE_BERRY_HEAL_FRACTION from include/constants/items.h * Moved CONFUSE_BERRY_HEAL_FRACTION back to include/constants/items.h * Unified item description strings where possible --------- Co-authored-by: Bassoonian <[email protected]>
Description
About 3 hours and a half ago, I was indirectly reminded of #3243.
Somehow, that made me think that having the text strings that compose an item's descriptions in an entirely separate file is a pita when you decide to add entirely new items, TM/HM or not.
The very first way to deal with that I thought was to use the
COMPOUND_STRING
macro that SBird added in #2733 to write the strings directly in the.description
field of each item.That worked.
Then I gauged the public opinion in the Discord server, and most of the people seemed to be on board with the idea.
So I proceeded to ditch every single dedicated text string for item descriptions and submit a PR to the expansion.
This PR will remain a Draft until #3284 and #3274 are merged. Personal choice. I don't wanna trouble kittenchilly 😆
Any early reviews would be appreciated.
Discord contact info
lunos4026