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

IO/config: parse from XML/JSON formatted string #309

Closed
wants to merge 3 commits into from

Conversation

roccoarpa
Copy link
Contributor

Added Config specialization class (ConfigStringParser) to read and write XML (or JSON) directly from and onto c++ string. The class is useful to exhange info without passing from filesystem each time.

Enhanced IO integration tests 2 e 4 to prove the capabilities of the new class.

@roccoarpa
Copy link
Contributor Author

@edoardolombardi FYI, since it's needed by mimic pull https://github.com/optimad/mimic/pull/386

@roccoarpa roccoarpa force-pushed the io.configuration.XML.parsing.from.string branch 2 times, most recently from 579f512 to 56ea949 Compare May 24, 2022 16:32
added Config specialization class (ConfigStringParser) to read and write
XML (or JSON) directly on c++ string. The class is useful to exhange  info without passing from filesystem each time.

Enhanced IO integration tests 2 e 4 to prove the capabilities of the new class.
@roccoarpa roccoarpa force-pushed the io.configuration.XML.parsing.from.string branch from 56ea949 to ecbd298 Compare May 30, 2022 08:22
src/IO/configuration.cpp Outdated Show resolved Hide resolved
\param rootConfig pointer to the Config tree to be stringfied.
\param rootname (optional) name of the root section. Default is "root".
*/
void writeBufferConfiguration(std::string &source, const Config *rootConfig, const std::string &rootname)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to add a private function that takes in input a xmlTextWriterPtr? This will allow to share code between writeConfiguration and writeBufferConfiguration? For "symmetry", this apply also to "readBufferConfiguration".

In bitpit , arguments that are modified by the function are usually passed by pointer at the end of the argument list.

Do we really need a function with a different name? The signature of writeConfiguration and writeBufferConfiguration is different, we can use the same name.

I would still pass a "format" argument (also in the read* function). In this way the function that write the configuration to file and the one that write the configuration to string will generate the same output.

Copy link
Contributor Author

@roccoarpa roccoarpa May 31, 2022

Choose a reason for hiding this comment

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

You're right a compact code is always better. But since pending pull #267 and needs of issue #269 suggest a major underhood rework of tree interface and its parsing capabilities, and the introduction of rapidXML will make a lot easier to compact the code w.r.t. to libxml2, I think it's not the case to lose much time on this now.

src/IO/configuration_JSON.cpp Outdated Show resolved Hide resolved
@@ -360,4 +361,113 @@ namespace config {

}

/*!
\class ConfigStringParser
Copy link
Member

Choose a reason for hiding this comment

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

Is this new class really needed? Can we just add an overload to the read/write functions of ConfigParser, this overloads will take in input the string and the format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing a class of its own, without overhead of filesystem checks, version control, propagation to static utilities and whatsover was faster and simpler. As I mention earlier, introducing rapidXML instead of libxml2 can straightforward the I/O part a lot.

get rid of boolean for a more clear enum to select the string preferred format
@roccoarpa
Copy link
Contributor Author

For other questions: i think is not worth doing the works asked right now, since we need to rework the class very soon anyway.
But if you are not 100% convinced, i can throw away this pull and move the current code elsewhere. Let me know.

@roccoarpa roccoarpa closed this Jul 1, 2022
@roccoarpa
Copy link
Contributor Author

moved elsewhere.

@roccoarpa roccoarpa deleted the io.configuration.XML.parsing.from.string branch July 1, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants