Support nested variable groups #537
Conversation
| } | ||
| return get_variable_group_variables(nestedDescriptions, element, connector); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't like to build up the nestedDescription map here. If the variable_goup_decsriptions in the variable_group_description struct (line 560) was a map instead of a vector, we could have this recursion without building up temporary maps.
There was a problem hiding this comment.
Agreed. Is there any reason to not make it a map?
There was a problem hiding this comment.
Don't think so. Just didn't want to make changes in the parser now. Let's do this as a separate refactoring task for the ´cse_config_parser´
|
Did an attempt on connecting variables within nested variable groups. |
ljamt
changed the title
There was a problem hiding this comment.
What has been added since the mob session looks good, though I do agree that it might be better to use a map for variable_group_description::variable_group_descriptions.
Marking this as "approved" despite the fact that I have some comments, since getting this in is urgent. Also, none of the issues are really critical. They can be taken care of during a future (inevitable, I'd say) clean-up of the entire parser. Fix if you have the time, otherwise we'll do it later.
|
|
||
| std::string variableName = tc(variableElement->getAttribute(tc("name").get())).get(); | ||
| vgd.variables.push_back(std::move(variableName)); | ||
| for (auto variableGroupChildElement = variableGroupElement->getFirstElementChild(); variableGroupChildElement != nullptr; variableGroupChildElement = variableGroupChildElement->getNextElementSibling()) { |
There was a problem hiding this comment.
There are some reeeeally long lines of code in this file, and it's actually a bit tricky to see the end of lines like this one due to how GitHub places the horizontal scroll bar.
I'm not going to suggest that we use this PR to reformat the entire file, but it would be nice if added/modified code was made a tad more readable. ;)
| } | ||
| return get_variable_group_variables(nestedDescriptions, element, connector); | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed. Is there any reason to not make it a map?
| @@ -562,6 +562,7 @@ struct variable_group_description | |||
| std::string name; | |||
| std::string type; | |||
| std::vector<std::string> variables; | |||
| std::vector<variable_group_description> variable_group_descriptions; | |||
There was a problem hiding this comment.
I think groups or subgroups would be a more descriptive name for this.
|
Leaving the comments unresolved for a coming refactoring task. |
Creating a PR of the status after last mob session.
Tests are failing as we need to create connections from nested variable groups
Closes #536