Refactor ConfigParser #236
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use a templated ConfigParam instead of a switch statement and "type" enum - Take advantage of yaml-cpps existing `convert` mechanism - Still need to implement `convert` for Eigen and CV matrices
Explicitly instantiate the functions that were there before
4 tasks
chutsu
reviewed
Nov 1, 2017
|
|
||
| */ | ||
| template <> | ||
| struct convert<cv::Mat> { |
There was a problem hiding this comment.
Was this not supposed to be in the vision module?
chutsu
reviewed
Nov 1, 2017
| /** Base class representing a parameter to be parsed in the yaml file */ | ||
| struct ConfigParamBase { | ||
| ConfigParamBase(std::string key, bool optional) | ||
| : key{std::move(key)}, optional{optional} {}; |
There was a problem hiding this comment.
chutsu
reviewed
Nov 1, 2017
| ConfigStatus loadParam(const ConfigParamBase ¶m); | ||
|
|
||
| /** Load yaml file at `config_file`. */ | ||
| ConfigStatus load(std::string config_file); |
There was a problem hiding this comment.
const std::string & maybe, I know I haven't been putting const everywhere but maybe we should?
chutsu
approved these changes
Nov 1, 2017
There was a problem hiding this comment.
Code looks good. I'm not sure why you didn't want to have the ability to load cv::Mat here?
I think more explanation is needed to say why wave_utils shouldn't have OpenCV as a dependency considering.
- It is should be installed by ROS by default
- The chances of any of us not using / having OpenCV on our deployment or dev machines is rather rare.
- Lastly,
cv::Matis very stable, and it is very unlikely to change in the future.
If you can update the issue about this problem I think it would be useful for future reference.
|
OK, I added some reasoning to the issue. One of our projects does currently have an issue with opencv which I can tell you about offline. |
leokoppel
force-pushed
the
refactor_configparser
branch
from
fbf617c to
3cceabc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prereq for #234
Refactor ConfigParser. The primary motivation is to allow defining parsing functions in other modules, so wave_utils does not depend on OpenCV, or any future classes we want to parse.
Basically this is a wrapper around yaml-cpp's
convertmechanism. However, by request I kept the interface to ConfigParser the same, so other classes still useaddParam,load(file), etc.Note that despite the templates, the parsing functions for Matrix, etc. are still compiled into the library. Only the same types are still supported.
Pre-Merge Checklist:
clang-format