Add reset methods for each option

[melissalinkert]

As discussed with @DavidStirling, there wasn't a way to reset options to their default value, and some of the setters contain validation logic that prevents the default value from being accepted (e.g. pyramidResolutions).

This PR adds a reset method corresponding to the get and set method for each option. resetOptions() calls each individual option's resetter for convenience. Open to other thoughts on how to implement this, what's in 5a99e2b was just the most straightforward change.

Once we're happy with changes here, raw2ometiff will need the same treatment. This may end up being what drives the 0.8.0 release.

[DavidStirling]

LGTM

[sbesson]

Biggest caveat from the current proposal is that the default value for every option are now defined in two separate places. If we go with this approach, I would be inclined to group the reset method with the companion setter to reduce the maintenance burden and the risks of being out-of-sync.

On the API itself, my reading of the release notes for picocli 4.6.3 is that the ability to reset all options should be built-in with the CommandLineImporter API for certain use cases.
I assume the main driver of this API addition is to be able to reset options individually? Do you have a code snippet or a branch showing how this API is intended to be consumed downstream?

[melissalinkert]

On the API itself, my reading of the release notes for picocli 4.6.3 is that the ability to reset all options should be built-in with the CommandLineImporter API for certain use cases.

https://picocli.info/#_annotating_methods_of_a_concrete_class also suggests that the picocli-friendly way to do this is to annotate each @Option with a defaultValue. The main reason I didn't do that initially is that it requires loosening the restrictions in some of the setters - the pyramid resolutions are a good example, where the default value is null, but the setter only accepts strictly positive integers.

b532b77 adds a test for resetting, which I should have done earlier. https://github.com/melissalinkert/bioformats2raw/commits/picocli-default-reset is a separate branch off that work that removes the resetters and uses the picocli-friendly defaultValue approach instead; both approaches can be compared side-by-side if desired. If that branch looks more sensible than what's here, I can close this PR and open a new one. If the separate resetter method is preferred, then I can reorder the methods so that the setter and resetter for each option are next to each other.

[sbesson]

Thanks for the extra commit for comparison. I have a slight preference for the picocli-default-reset option in terms of maintenance. A side-advantage of this new approach is that it also unifies the usage of defaultValue across all option setter methods.

@DavidStirling back to you, could you check whether the API changes proposed in #219 (comment) are sufficient to meet your requirements? Off-hand the only thing that seems lost from the current PR might be the ability to reset options individually to their default value.

[DavidStirling]

Resetting options individually isn't strictly critical. The one issue I have is how to access the picocli default values outside of a cli context. As it stands NGFF-Converter isn't handling Converter objects within a CommandLine wrapper (since we no longer want to use the CLI directly). I'm not sure I see a simple way of resetting aConverter instance without the wrapper. Ideally the defaults need to be intrinsic to the Converter itself rather than the CLI decorator.

The final reset method needs to live here. Currently we blow away the entire Converter instance and re-apply the program-specific defaults.

What would also be really useful is if all the setters were all capable of accepting the default value (i.e. not rejecting null when supplied to int settings). There are some instances ("Apply to all") where I'd like to copy settings directly from one instance to another. Looks like a couple of those setters are fixed on the linked branch.

[melissalinkert]

The one issue I have is how to access the picocli default values outside of a cli context.

Is it a requirement to access default values separately? Or is resetting to defaults and then calling getters (which should work with either branch) sufficient?

What would also be really useful is if all the setters were all capable of accepting the default value

https://github.com/melissalinkert/bioformats2raw/commits/picocli-default-reset does change all of the setters to accept the default values. That approach means that every setter gets called when the options are populated, so all setters must accept the default value by definition. In command line usage, each setter is either called with the specified command line argument, or the defaultValue if no corresponding argument was provided. In API usage (see the tests), all of the setters are called with their defaultValue initially, and then can be overridden via setters before conversion is started.

I don't see an obvious way to reconcile all of these points:

1. Not require use of CommandLine wrapper
2. Only define defaults once
3. Use picocli's built-in defaultValue annotations

so we probably need to agree on which ones take priority. It sounds like https://github.com/melissalinkert/bioformats2raw/commits/picocli-default-reset is closer to ideal that what's open here, so unless any objections I'll plan to close this and open a new PR with that branch next week.

[joshmoore]

If I can pour fuel on the fire (with the caveat that I haven't dug into Converter's code in some time), but another goal I would generally like to have on the list is the re-usability of the code as a library rather than a CLI. I know when I was previously looking at making changes, I (briefly) explored the idea of moving the options to an option class. That would certainly make reseting the values trivial, but likely plays least well with Picocli??

[DavidStirling]

Is it a requirement to access default values separately? Or is resetting to defaults and then calling getters (which should work with either branch) sufficient?

We can probably get away without direct access, you're right there. I'd prioritise making things convenient for library usage over Picocli's expectations as Josh said.

[melissalinkert]

As discussed earlier today, closing this in favor of #226 (i.e. the branch noted above). I'm not opposed to adding a more sophisticated options API in the future, but for the moment let's focus on incremental improvement that helps our immediate needs. #226 as noted above does update the setters to accept default values (unlike what's here).