refactor(commands)!: Migrate command plugins to new plugin API #9507
Conversation
mnonnenmacher
force-pushed
the
cli-plugins
branch
2 times, most recently
from
5f22e1c to
f1a85ce
Compare
cli/src/main/kotlin/OrtMain.kt
Outdated
| @@ -123,7 +124,8 @@ class OrtMain : CliktCommand(ORT_NAME) { | |||
| helpFormatter = { MordantHelpFormatter(context = it, REQUIRED_OPTION_MARKER, showDefaultValues = true) } | |||
| } | |||
|
|
|||
| subcommands(OrtCommand.ALL.values) | |||
| // Commands are not configurable so we can pass an empty PluginConfig here. | |||
There was a problem hiding this comment.
Nit: I guess we should use passive voice here, too. Like "Pass an empty PluginConfig here as commands are not configurable."
There was a problem hiding this comment.
Should we make the code self-explanatory to remove the need for this comment?
I guess the comment would be repeated over and over again.
We can put a constant PluginConfig.EMPTY, or similar, which would also avoid unnecessary instance creation.
There was a problem hiding this comment.
I guess the comment would be repeated over and over again.
Do you mean for other plugin types? I'm not sure about that, as commands are really special in this regard.
However, I was also wondering whether we should allow factory.create() to take no arguments for this case instead of introducing PluginConfig.EMPTY.
There was a problem hiding this comment.
I meant writing here: subcommands(OrtCommandFactory.ALL.map { (_, factory) -> factory.create(PluginConfig.EMPTY) })
And drop the code comment.
There was a problem hiding this comment.
Understood. But my question was: Where do you potentiality see the comment to "repeated over and over again"? In upcoming plugin migrations?
There was a problem hiding this comment.
I didn't have any specific callers in mind. I just thought that this comment would need to be repeated for each create() call which passes PluginConfig() for consistency.
There was a problem hiding this comment.
I have updated the comment to not use passive voice.
However, I was also wondering whether we should allow factory.create() to take no arguments for this case instead of introducing PluginConfig.EMPTY.
I'd rather not introduce that option before all plugins are migrated to avoid forgetting to pass the config.
I meant writing here: subcommands(OrtCommandFactory.ALL.map { (_, factory) -> factory.create(PluginConfig.EMPTY) })
And drop the code comment.
That code still does not explain why it is ok to pass an empty config so I would keep the comment anyway.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9507 +/- ##
=========================================
Coverage 68.00% 68.00%
Complexity 1291 1291
=========================================
Files 249 249
Lines 8819 8819
Branches 917 917
=========================================
Hits 5997 5997
Misses 2433 2433
Partials 389 389
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cli/src/main/kotlin/OrtMain.kt
Outdated
| @@ -123,7 +124,8 @@ class OrtMain : CliktCommand(ORT_NAME) { | |||
| helpFormatter = { MordantHelpFormatter(context = it, REQUIRED_OPTION_MARKER, showDefaultValues = true) } | |||
| } | |||
|
|
|||
| subcommands(OrtCommand.ALL.values) | |||
| // Commands are not configurable so we can pass an empty PluginConfig here. | |||
There was a problem hiding this comment.
Should we make the code self-explanatory to remove the need for this comment?
I guess the comment would be repeated over and over again.
We can put a constant PluginConfig.EMPTY, or similar, which would also avoid unnecessary instance creation.
Migrate the command plugins to the new plugin API. Relates to #9403. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
cli-plugins
branch
from
f1a85ce to
2e60641
Compare
Migrate the command plugins to the new plugin API.
Relates to #9403.