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 fmt::format instead of SString constructor for string formatting #1804

Closed
wants to merge 1 commit into from

Conversation

Pirulax
Copy link
Contributor

@Pirulax Pirulax commented Nov 5, 2020

Replace all SString consructors that are used for string formatting to fmt::format.
Replace SString ctors that are used for number -> string conversion with std::to_string.

Todo:

  • Rewrite SString constructor to use fmt::format as well.

I've made a python script (250-300 lines, terrible code), that did all this for me, thankfully.

@ccw808
Copy link
Member

ccw808 commented Nov 5, 2020

Might cause an issue with localization

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 5, 2020

And how could they be solved?

Edit: Searched it up on google, and found this: fmtlib/fmt#305 . Interestingly the doc doesnt say anything about 'n'

SString strBuffer(_("Connecting to %s at port %u failed!"), m_strHost.c_str(), m_usPort);
SString strBuffer(_("Connecting to {} at port {} failed!"), m_strHost, m_usPort);
Copy link
Member

Choose a reason for hiding this comment

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

Strings inside _() are matched and replaced depending on the selected language. e.g. "Connecting to %s at port %u failed!" is replaced with "تعذر الإتصال بـ %s في المنفذ %u!" when Arabic is selected. Changing the content of the original string will make the match fail, and probably flag the string as requiring a new translation at https://translate.mtasa.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was done by a script, and I didnt verify it yet. (Forgot to mark the PR as draft).
I'll correct this, thanks!

Edit: Can I edit the .po files myself, to make it work? Eg.: Replace Connecting to %s at port %u failed! with Connecting to {} at port {} failed! in the .po as well.

(Reposted it, as edits dont show up on dc)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I edit the .po files myself

The files are generated by one of the scripts here https://github.com/multitheftauto/mtasa-blue/tree/master/utils/src I think build_gettext_catalog.py

@Pirulax Pirulax marked this pull request as draft November 5, 2020 22:08
Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

As with #1804 I think we should wait for c++20 instead of adding another dependency.

Especially hopping from SString to fmt::format to std::string seems like a lot of unnecessary mass changes.

@Pirulax Pirulax force-pushed the use-fmt branch 3 times, most recently from ca720cf to 85ac059 Compare November 8, 2020 19:17
@Pirulax Pirulax closed this Nov 8, 2020
@Pirulax Pirulax reopened this Nov 8, 2020
@Pirulax Pirulax force-pushed the use-fmt branch 2 times, most recently from 37f715a to e1bfe90 Compare November 8, 2020 22:54
@qaisjp
Copy link
Contributor

qaisjp commented Nov 12, 2020

"Set cppdialect to C++20 #1803" has been closed in favour of just waiting for C++20 to release. Going to close this too.

@qaisjp qaisjp closed this Nov 12, 2020
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.

3 participants