Command Line Modules not handling empty strings correctly when passing to back end.

[MattClarkson]

If an argument requires a string, then an empty string is invalid when passed to the LocalProcess back end.

i.e. if you pass --myParameter1 --myParameter2 Banana

and the command line program expects something between --myParameter1 and --myParameter2, it will fail. The solution should be that if no text is specified, the text --myParameter1 is not passed either.

[MattClarkson]

b085c99

[finetjul]

Shouldn't the following be passed ?

--myParameter1 "" --myParameter2 Banana

I can't think of an issue right now, but I think there might be some danger to consider "argument not being passed' and "empty argument" to be the same.

[MattClarkson]

My original intention was to stop command line modules crashing when the parameter flag was passed, and the command line module then expected a value that was not passed. The SlicerExecutionModel project generates a command line parser that either expects an argument not to be passed, or it to be passed along with its value. So, while I can see your point, this code change seems to be correct in terms of what the underlying command line module expects.

[finetjul]

I'm sure your fix works fine with SlicerExecutionModel.
I am just questioning for the "best" (has more semantic meaning) solution to convey the idea you want your parameter1 to be an empty string:
a)

--myParameter1 "" --myParameter2 Banana

or b)

--myParameter2 Banana

I would vote for a) as it is more verbose and more explicit.
But maybe I'm just being over cautious here...

[MattClarkson]

Sure. I don't know. I will await Sascha's opinion as it currently affects MITK users and NifTK users. I would vote for merging this change, as the current implementation just crashes.

Im happy to take any advice though.

[jcfr]

Agree with Julien.

If an empty string is valid, you could set the parameter as optional with
an empty string as default value.

Jc

Sent from my mobile device
On Mar 13, 2013 10:02 AM, "Matt Clarkson" notifications@github.com wrote:

Sure. I don't know. I will await Sascha's opinion as it currently affects
MITK users and NifTK users. I would vote for merging this change, as the
current implementation just crashes.

Im happy to take any advice though.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/307#issuecomment-14846224
.

[saschazelzer]

@MattClarkson Thanks for working on this one!

I actually also would prefer option a) . I think we should just pass all parameters to the command line module. While we could omit the arguments where the user did not change the default value, I don't see much value in it.

Talking about empty arguments, I think we should also just always quote all passed arguments, in case there are spaces or special characters.

[finetjul]

I agree with Sascha, all the arguments should probably be quoted.

[MattClarkson]

OK. Thanks for feedback. The original problem was strings not being passed to back end at all. This was fixed in #306. Then there was the fact that empty strings were not passed, which was the original intention of #307. This was raised as a separate issue, as once #306 was fixed, it was realised that empty strings caused this:

--myParameter1 --myParameter2 Banana

so after Juliens (and others) suggestions, it now passes:

--myParameter1 "" --myParameter2 Banana

In my case, I don't actually have command line modules that pass strings, so I admit my testing was a little too narrow. The current solution is now better than it was before #306 and #307. However, I have had numerous problems with trying to get strings with quotes properly escaped. So, to be pragmatic, and make progress, I have merged to master, and will raise myself a separate ticket to investigate proper escape sequences on the 3 major platforms.

[saschazelzer]

Sounds like a great plan to me :-)

[MattClarkson]

Hi Sascha, thanks for this. I just tested our app on Linux using:

Empty list
1 space
3 spaces
Space asterisk space
Asterisk space asterisk
hello
hello space
Space double quote space
Space double quote space double quote
Double quote space double quote

and can confirm that CTK passes to the command line module the correct string.

I think my confusion stems from the fact that I was testing with a bash script as a command line module, so it was the bash script that was reporting the strings incorrectly.

Sorry.