Barclay could throw a more helpful error message when trying to populate an immutable list argument field.

[jamesemery]

I noticed that I get a java.lang.UnsupportedOperationException from CommandLineArgumentParser when I have an argument using an immutable list like from Collections.emptyList(). This throws the following unhelpful stack-trace segment:

Caused by: java.lang.UnsupportedOperationException
	at java.util.AbstractList.add(AbstractList.java:148)
	at java.util.AbstractList.add(AbstractList.java:108)
	at org.broadinstitute.barclay.argparser.CommandLineArgumentParser.setArgument(CommandLineArgumentParser.java:697)
	at org.broadinstitute.barclay.argparser.CommandLineArgumentParser.parseArguments(CommandLineArgumentParser.java:418)
	at org.broadinstitute.hellbender.cmdline.CommandLineProgram.parseArgs(CommandLineProgram.java:217)
	at org.broadinstitute.hellbender.cmdline.CommandLineProgram.instanceMain(CommandLineProgram.java:191)
	at org.broadinstitute.hellbender.Main.runCommandLineProgram(Main.java:131)

Since there is no obvious indication of what field is causing the error, this should either throw a more helpful exception pointing to which argument caused the problem or there should be a check that all the requested argument list fields are mutable.

[droazen]

(or barclay could just make the Collection mutable by making a mutable copy)

[magicDGS]

@droazen, I think that it is safer to disallow immutable collections as default values, because the user may specified it like that as a requirement. If they really need it immutable, they should make their copy by themselves after parsing.

If in the case of this issue, using Collections.emptyList() could be easily substituted by new ArrayList(), and I guess that the user does not need immutability here.

I'll do a PR to catch the UnsupportedOperationException for handling collections to throw an internal parser exception to be sure that mutability is expected by the user.

[droazen]

I'm not sure it makes sense to have an immutable value for a command-line argument. If some tool setting is meant to be immutable, the author should make it a constant rather than a barclay argument. If an immutable collection is being used, it's almost certainly by accident, which is why I think barclay should just auto-convert to a mutable one. @cmnbroad what do you think?

[magicDGS]

But if it is by accident, it is better from my point of view to make the user aware that he did it. If not, it is silently doing the conversion. That's why I prefer to throw to make aware the author that he should make it a constant rather than a Barclay argument. See #102 for my proposed solution.

[cmnbroad]

I think I'd prefer to throw in this case. Otherwise we need to figure out whether or not to preserve existing values based on the intersection of APPEND_TO_COLLECTIONS, and null argument values used to clear. These cases need to be handled in this PR either way, but I think it will be simpler if we just throw.

[droazen]

Alright, makes sense 👍