Bugfix: enable variable serialisation in unnamed component #71
Conversation
|
good catch @hsorby :) the new tests and fixed implementation look good to me. |
| "<component>" | ||
| "<variable name=\"var1\" units=\"dimensionless\"/>" | ||
| "<variable name=\"var2\"/>" | ||
| "<variable name=\"var3\"/>" |
There was a problem hiding this comment.
It might be worth having one of these with only units, or only initial_value, and no name, so more combinations are checked?
There was a problem hiding this comment.
+1
Otherwise, I would indent the 'variable' lines.
There was a problem hiding this comment.
Good idea, @jonc125 , as a way to pick up more permutations in a single test. This bugfix pull actually pre-dates the addition of initial values so will add those in under that pull set. :-)
|
Besides the above minor comments and Jonathan's suggestion, it all looks good to me too. |
|
|
||
| libcellml::UnitsPtr u = std::make_shared<libcellml::Units>(); | ||
| u->setName("dimensionless"); | ||
| v1->setUnits(u); | ||
| v3->setUnits(u); |
There was a problem hiding this comment.
Tabs instead of spaces issue?
There was a problem hiding this comment.
Good catch- fixed in d5ddf20
|
So I believe all issues here are resolved, so I'll merge it in to avoid conflicts in #68. |
@hsorby highlighted a bug from pull #64 by submitting a failing test. A nested if statement prevented serialisation of variables in unnamed components.
Relevant to issue #63