Closed melissalinkert closed 1 year ago
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.
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 https://github.com/glencoesoftware/bioformats2raw/pull/219#issuecomment-1773208406 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.
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.
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:
CommandLine
wrapperdefaultValue
annotationsso 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.
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??
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.
As discussed earlier today, closing this in favor of https://github.com/glencoesoftware/bioformats2raw/pull/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).
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 theget
andset
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.