Skip to content

Let AbstractOptions implement Map<String, Object>#3

Open
frauzufall wants to merge 1 commit intomasterfrom
abstractoptions-map
Open

Let AbstractOptions implement Map<String, Object>#3
frauzufall wants to merge 1 commit intomasterfrom
abstractoptions-map

Conversation

@frauzufall
Copy link
Member

@frauzufall frauzufall commented Apr 24, 2020

  • this makes it possible to use options with the scijava
    CommandService.run(final Class<C> commandClass, final boolean process, final Map<String, Object> inputMap) method:
ImageJ ij = new ImageJ();
ij.command().run(MyCommand.class, true, new MyOptions().setSomething(true));

@frauzufall frauzufall mentioned this pull request Apr 24, 2020
Open
@frauzufall
AbstractOptions: implements Map<String, Object> #1
c5a0e59 
- this makes it possible to use options with the scijava
CommandService.run(final Class<C> commandClass, final boolean process,
final Map<String, Object> inputMap) method
@frauzufall frauzufall force-pushed the abstractoptions-map branch from 474dd23 to c5a0e59 Compare April 24, 2020 14:09
@frauzufall
Copy link
Member Author

frauzufall commented Apr 24, 2020

Should I rather remove theOptions and extend LinkedHashMap?

@frauzufall frauzufall requested a review from tpietzsch May 18, 2020 21:11
@tpietzsch
Copy link
Member

tpietzsch commented Jun 11, 2020 •
edited
Loading

The problem with this is that default values (options that were not set) don't show up in the map.
There is now an (optional) forEach(BiConsumer<String, Object>) method that can be used to harvest all values, including defaults.

How does this play with the command service inputMap? Do the commands also have default parameters that potentially clash with the defaults from the AbstractOptions?

If that is not the case:
The Map interface could be implemented on top of forEach, but would for example for every get need to run a complete forEach over every option.
Alternatively, a Map can be explicitly build by running forEach once. But this must be explicitly triggered by, for example, an AbstractOptions.asMap() method. This would also have the advantage of not cluttering the auto-complete on Options classes. Do you think that would be a feasible solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tpietzsch tpietzsch Awaiting requested review from tpietzsch

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@frauzufall @tpietzsch