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

Replace F extension with string interpolation #19372

Merged
merged 2 commits into from
May 8, 2021

Conversation

teinarss
Copy link
Contributor

No description provided.

@pchote
Copy link
Member

pchote commented Apr 24, 2021

There are a bunch of places (e.g. the first several exception changes in the files view) where this is inserting arbitrary newlines that IMO hurt readability more than they help. Can we please unstage those?

@teinarss
Copy link
Contributor Author

Updated.

@teinarss
Copy link
Contributor Author

teinarss commented Apr 30, 2021

Maybe reconsider this one - there is an option even better than what ReSharper implies. https://stackoverflow.com/a/63904747

The only places where we are using {x:x2} style is in none hot paths, as far as i can see, and this PR doesn't introduces any changes with valueType.ToString(format) to {valueType:format}, which causes the boxing.

@dragunoff
Copy link
Contributor

How would this work with localization? 🤔

@pchote
Copy link
Member

pchote commented May 3, 2021

The translation library has its own string replacement system, so won't interact with this at all.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

I still think its really awful when string interpolations include arbitrarily complex expressions, but I don't think it is productive to get hung up over that here. The majority of cases in this PR are simple variables and are a clear improvement, and the ugly cases can be cleaned up the next time someone touches the impacted code.

LGTM except for a couple of small nits:

OpenRA.Game/Network/GameServer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
OpenRA.Game/Scripting/ScriptMemberExts.cs Outdated Show resolved Hide resolved
OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Graphics/Voxel.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Util.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Util.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyUtils.cs Outdated Show resolved Hide resolved
@teinarss
Copy link
Contributor Author

teinarss commented May 5, 2021

Updated.

OpenRA.Game/Game.cs Outdated Show resolved Hide resolved
@teinarss
Copy link
Contributor Author

teinarss commented May 6, 2021

Fixed.

pchote
pchote previously approved these changes May 7, 2021
@reaperrr
Copy link
Contributor

reaperrr commented May 8, 2021

Needs rebase, sorry. I guess we should merge this before any other PRs it might conflict with.

@teinarss
Copy link
Contributor Author

teinarss commented May 8, 2021

Updated.

@reaperrr reaperrr merged commit 0c50057 into OpenRA:bleed May 8, 2021
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.

None yet

5 participants