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

Use Core/Scene stringnames consistently #91909

Merged
merged 1 commit into from
May 14, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 13, 2024

Follow-up to #81303
Replaces #50941

This PR solidifies usage of CoreStringNames and SceneStringNames by replacing every string instance by its singleton equivalent, if it already existed. I also removed some duplicate or unused names. I might have missed some very common names, as it's difficult to find them in all replaceable contexts.

Up to discussion, but I included core string names in variant.h and scene string names in node.h and resource.h. This makes them easily accessible without extra includes. These files are not modified often, so it's probably fine to do that. I removed all other includes of these files. I didn't do the same for EditorStringNames, because they are included in much more files and there is no single file that's included in all editor code.

Note that it's rather unreasonable to enforce using singleton string names when available,. We could just replace some newly added strings from time to time. Maybe I'll make a helper script for that.

@KoBeWi KoBeWi added this to the 4.x milestone May 13, 2024
@KoBeWi KoBeWi requested review from a team as code owners May 13, 2024 15:03
core/io/resource.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member Author

KoBeWi commented May 13, 2024

Ok I don't know where to put scene string names include :/
Maybe it could stay in node.h, so at least some files won't need it. But otherwise there is no way to avoid including it everywhere.

@KoBeWi KoBeWi force-pushed the have_fun_reviewing_this branch 2 times, most recently from f455155 to 794e242 Compare May 13, 2024 21:22
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Were planning to do one of these myself, and looks great!

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Didn't review in depth, but makes sense to me.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 14, 2024

I wrote some simple Ruby script that scans the codebase for StringNames:
https://pastebin.com/ZaYn57YP
It can detect unused or underused singleton StringNames, duplicate names and suggest replacement for raw strings. Though it needs smarter filters/exceptions, because right now it spams over 2k Strings 🤔

EDIT:
This PR seems to have reduced the spam by 700.

@akien-mga akien-mga merged commit bdefe53 into godotengine:master May 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants