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

Duplicate code for parsing INI files #515

Open
Zervox opened this issue Apr 3, 2017 · 4 comments
Open

Duplicate code for parsing INI files #515

Zervox opened this issue Apr 3, 2017 · 4 comments
Labels
Milestone

Comments

@Zervox
Copy link
Contributor

Zervox commented Apr 3, 2017

I noticed that there is already a Parser set in Txt, toInt, toDouble and toBool, could be moved from Ini::Value into Parser as static with const std::string, replacing all Value with std::string instead.(no point having a duplicate structure for something which could be called as generics.

@phobos2077
Copy link
Contributor

What is the real benefit of this change? Is there any real issue? Also note that Ini::Value holds a union of various types, not just std::string so you can't trivially replace it.

@Zervox
Copy link
Contributor Author

Zervox commented Apr 4, 2017

benefits:
less inclusions for getting the same functionality which Txt files still uses from Ini::
Ini::Value holds no union of types, it holds a string and performs the same operations Txt::Parser does, it converts a std::string to value or to array, making toInt, toDouble and toBool static.
merging these two structures' functionality actually is trivial
cons:
Haven't found any, runs exactly the same, no performance differnce and no data difference, no loss of functionality.

@phobos2077
Copy link
Contributor

phobos2077 commented Apr 5, 2017

There are actually 2 Ini::Value classes in falltregeist::Ini and falltergeist::Format::Ini namespaces, basically duplicated functionality. The first was written long time ago to just read the settings for falltergeist itself, the second was created specifically for parsing game text formats. I don't see any duplicated functionality within each of these two. Are you sure you're not mixing them up?

So the actual issue is to maybe get rid of Falltergeist::Ini classes altogether and use the one from Formats instead...

@phobos2077 phobos2077 changed the title Ini Value Duplicate code for parsing INI files Apr 7, 2017
@phobos2077 phobos2077 added the code label Apr 7, 2017
@phobos2077 phobos2077 added this to the 0.9.0 milestone Apr 7, 2017
@667bdrm
Copy link
Contributor

667bdrm commented Jul 12, 2020

city.txt parser has useful logic for getting array of properties by simple regilar expression. (example: entrance_%d) Moving this to common parser could be useful in future.

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

No branches or pull requests

3 participants