On 14/10/2016 23:26, Alexandre (Shura) Iline wrote:

Could you please take another look?

I have added more options, and fixed other things you have pointed out. I have 
also picked up a couple more tests to cover the newly added methods.


The usages are easy to read so it looks to me that this effort is coming along well.

On repeating options again then this version is an improvement but it still doesn't add to the list for usages like .addExports(...).addExports(...). So rather than taking a String[] then I think it would be simpler to take a String for one value and just add to the builder's list.

I see shouldContain requires specifying the OutputKind when sometimes the test doesn't care if the output is sent to stdout or stderr, maybe there can be a shouldContain overload that searches both streams (we have this in ProcessTools already).

For the updated tests then I assume `throws Exception` can be removed as there are no unchecked exceptions now. Related is whether TaskError should be TaskException extends RuntimeException instead.


Reply via email to