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

Move STR_NONE, STR_EMPTY to StringIdType.h #22019

Merged
merged 1 commit into from May 15, 2024

Conversation

AaronVanGeffen
Copy link
Member

@AaronVanGeffen AaronVanGeffen commented May 11, 2024

This entirely removes the need for StringIds.h from a few often-used headers, notably GameActionResult.h and ResultWithMessage.h.

I suggest we rename STR_NONE to something like kStringIdNone in another PR. We can discuss the new name first.

@ZehMatt
Copy link
Member

ZehMatt commented May 11, 2024

I don't like common.h generally speaking, doing a common header violates almost always the rule of include what you need. I would rather have it StringId.h (the type and defaults like invalid) and StringIds.h (the collection/base ids)

@AaronVanGeffen
Copy link
Member Author

AaronVanGeffen commented May 11, 2024

Hmm, yes, I was also thinking it would be better to split it into a header for the Money types. I think StringId.h is a bit too similar to StringIds.h, but I would be fine with splitting it off to e.g. StringTypes.h?

Though tbh, it'll create quite the ripple effect, having to re-evaluate inclusion of common.h everywhere. Maybe we could do that in follow-up PR?

@ZehMatt
Copy link
Member

ZehMatt commented May 12, 2024

Hmm, yes, I was also thinking it would be better to split it into a header for the Money types. I think StringId.h is a bit too similar to StringIds.h, but I would be fine with splitting it off to e.g. StringTypes.h?

Though tbh, it'll create quite the ripple effect, having to re-evaluate inclusion of common.h everywhere. Maybe we could do that in follow-up PR?

Well for the time being you could just add the StringId.h include in common.h, I don't know about the plural form of StringTypes.h, we just got StringId at the moment, if one includes the wrong header one will figure that out soon enough.

My primary concern is that defining types in common.h makes code exploration quite difficult, if someone looks for a string type its somewhat expected to have that in the file name.

@AaronVanGeffen
Copy link
Member Author

AaronVanGeffen commented May 12, 2024

Well for the time being you could just add the StringId.h include in common.h, I don't know about the plural form of StringTypes.h, we just got StringId at the moment, if one includes the wrong header one will figure that out soon enough.

I wouldn't mind including the new header in common.h for the moment, but I'd really like to avoid having a StringId.h for the type and StringIds.h for the enum.

With StringTypes.h, I was thinking we could move some types from String.hpp in there as well. I suppose that's a different kind of string types, though.

Maybe we could bring this to a vote:
🚀 StringType.h 👍 StringTypes.h 🎉 StringIdType.h?

@AaronVanGeffen
Copy link
Member Author

The votes for StringIdType.h have it, so I moved the definition to a new file using that name.

@AaronVanGeffen AaronVanGeffen changed the title Move STR_NONE to common.h Move STR_NONE, STR_EMPTY to StringIdType.h May 14, 2024
@AaronVanGeffen AaronVanGeffen added this to the v0.4.12 milestone May 15, 2024
@AaronVanGeffen AaronVanGeffen merged commit 2f68c7e into OpenRCT2:develop May 15, 2024
22 checks passed
@AaronVanGeffen AaronVanGeffen deleted the move-str-none branch May 15, 2024 18:29
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.

None yet

2 participants