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

Handle newlines in cIniFile #5447

Merged
merged 7 commits into from Mar 22, 2023
Merged

Conversation

MacaylaMarvelous81
Copy link
Contributor

This pull request replaces \n with newline a newline when reading an ini file. When writing to an ini file, the newline will be replaced with \n.

Example 1:

# settings.ini
# ...
[Server]
Description=Line 1\nLine 2
# ...

issue5026-1

Example 2:

--- ...
local iniFile = cIniFile()

iniFile:SetValue("Key", "ValueName", "My Line 1\nMy Line 2")
iniFile:WriteFile("test.ini")
--- ...
# test.ini
[Key]
ValueName=My Line 1\nMy Line 2

Fixes #5026

When reading the ini file, replace \n with newline. When writing,
replace the newline with \n.
@NiLSPACE
Copy link
Member

NiLSPACE commented Oct 7, 2022

Isn't including regex for this overkill? Wouldn't a myString.replace("\\n", "\n") work as well?

@12xx12 12xx12 self-requested a review October 17, 2022 21:44
@12xx12
Copy link
Member

12xx12 commented Oct 18, 2022

Thank you for your contribution!

Firstly, please add yourself to the CONTRIBUTORS file to acknowledge our license and that your code is used in accordance to that.

Secondly I agree with @NiLSPACE that regex is a bit overkill.

Thirdly: Maybe we should add a sentence here, that line breaks are now supported and how they work

@MacaylaMarvelous81
Copy link
Contributor Author

Should I use ReplaceString from StringUtils instead of regex?

@12xx12
Copy link
Member

12xx12 commented Oct 24, 2022

Yes, please

@bearbin
Copy link
Member

bearbin commented Oct 24, 2022

Indeed, regex is a bit overkill, ReplaceString is a good alternative.

Also, please make sure it's possible to write a literal \n in the config! I suggest matching on \\n and replacing with \n (and the same in reverse). If we had other special characters to escape, replacing \\ with \ might make sense but just matching \\n makes most sense here to avoid mangling data unnecessarily.

@12xx12
Copy link
Member

12xx12 commented Oct 25, 2022

With the constraint mentioned, maybe regex is the way to go. If we want to add more for whatever reason, this is the easiest way to extend.

@MacaylaMarvelous81
Copy link
Contributor Author

Sorry for taking forever to push.. I'm having trouble with pushing to my fork

@12xx12 12xx12 requested a review from bearbin October 28, 2022 13:52
Copy link
Member

@bearbin bearbin left a comment

Choose a reason for hiding this comment

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

This doesn't address my comment made earlier; but I'd be happy to merge without that fix.

I do want the duplicated variables sorted out, no reason to have them.

src/IniFile.cpp Outdated
@@ -191,6 +193,7 @@ bool cIniFile::WriteFile(const AString & a_FileName) const
// Normally you would use ofstream, but the SGI CC compiler has
// a few bugs with ofstream. So ... fstream used.
fstream f;
AString runvalue, writevalue;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the point of having two separate strings here is - since writevalue is always set to the value of runvalue! Same for rawvalue up above.

Please remove one.

@12xx12 12xx12 requested a review from bearbin March 21, 2023 08:04
@12xx12
Copy link
Member

12xx12 commented Mar 21, 2023

I Fixed your request @bearbin

@12xx12 12xx12 merged commit fddbf65 into cuberite:master Mar 22, 2023
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.

Make cIniFile properly parse newline characters
4 participants