-
Notifications
You must be signed in to change notification settings - Fork 793
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
Add support for XML environmental variables parsing [10697] #1517
Add support for XML environmental variables parsing [10697] #1517
Conversation
Signed-off-by: Mauro <mpasserino@irobot.com>
ef0b37c
to
54ef93b
Compare
Hi, sorry for resurrecting this old PR. |
@irobot-ros Just that we did not have time to review it. Will do it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is a nice-to-have feature, it cannot be merged as-is.
First, it is only adding the possibility to use an environmental variable on pure string values. I think we should replace every tinyxml2::XMLElement::GetText
method call with a call to an auxiliary method receiving a tinyxml2::XMLElement*
and returning a const char*
, that will be the one performing the environmental variable access.
Methods XMLParser::getXMLInt
, XMLParser::getXMLUint
, and XMLParser::getXMLBool
should also include environmental variable support.
I also think that using a special character (like $
) to clearly indicate the environmental variable should be used is desirable.
Using method std::getenv
gives a warning on Windows about that method being unsafe.
Finally, adding this new feature should add tests and documentation for it. I suggest extending the unit tests on this file, and adding the corresponding PR to the documentation repo
Hi @irobot-ros, The base branch for this PR ( Sorry for the inconvenience! |
Sure, sorry for keeping this PR open for so long but we didn't have time to fully implement it as requested. |
I am closing this PR, though the feature is something which will be interesting to have in Fast DDS. I have opened #3377 to keep track of this feature. Also, this PR is targeting a branch that is no longer alive. A PR targeting master with this feature can be opened in the future. |
#3841 |
This commit would allow to use OS environmental variables in XML profiles. For example: