Fix deserialization multiple args #77
Merged
Conversation
This file contains hidden or 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
jcfr
changed the title
Member
|
FixDeserialazation -> FixDeserialization |
Member
|
could you specify in the title of the commit that it is realated to the param serializer. E.g: |
| { | ||
| w | " if (parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"].asBool())" | ||
| | " {" | ||
| | " deserializedVectorFlaggedArgs.push_back(\"" + flag + "\");" |
Member
There was a problem hiding this comment.
When you write
Multiple arguments can either be flagged or positional
do you mean
Argument described using "multiple=true" can either be flagged or positional
Contributor
Author
There was a problem hiding this comment.
Yes. I'll fix that.
Member
There was a problem hiding this comment.
When an multiple argument is present ...
->
Argument described using "multiple=true" are found in both JSON and in the command line, similarly to other arguments, the values of the JSON are ignored.
Member
|
Tests are failing with |
Member
|
Nice work overall 👍 After fixing the remaining issues, we should be in good shape |
The longflag has a higher priority over the flag.
…alization The code wasn't handling multiple parameters correctly and crashing when trying to deserialize arrays of arrays. This commit addresses that. Arguments using "multiple=true" can either be flagged or positional. - When they are positional, they must be the last argument of the CLI. In this case, the infrastructure already handles this. - When they are flagged, we need to detect that the argument allows for multiple flag/value pairs. To do so, we introduce the map called deserializedMultipleArgsMap. It stores a map of flag to vector of values. Json can populate that map based on the values it reads. Otherwise the command line parsing will populate this map. When an argument using "multiple=true" is present in the JSON and in the command line, simirarly than with other arguments, the values of the JSON are ignored. Testing for all the types that support the multiple argument is added.
The JSONModuleDescription did not escape the already escaped quote marks correctly, causing build error: error: expected ';' after top level declarator " \"Default\" : \"\\"1\\"\",\n" Escaping the escape character fixes this. A parameter with a string default with quotes is also added to testing to make sure that this works. Reported-by: Andrey Fedorov <fedorov@bwh.harvard.edu> Tested-by: Andrey Fedorov <fedorov@bwh.harvard.edu> Thanks: Andrey Fedorov <fedorov@bwh.harvard.edu>
johan-andruejol
force-pushed
the
FixDeserialazation-MultipleArgs
branch
from
d607f3b to
768beb7
Compare
Contributor
Author
|
@jcfr: Thanks for all the comments ! I uploaded an updated version with all the fixes you suggested. |
Member
|
Thanks 👍 |
Member
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Following the concerns raised in the Slicer's #576 (Slicer/Slicer#576), this addresses:
This also adds more testing and factorizes some of the testing infrastructure to be more easily extendable.
@jcfr, @thewtex: PTAL