New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added to check if the driver is invalid #1933
base: main
Are you sure you want to change the base?
Conversation
Thank you @workdone0 for the contribution. Will you also be able to merge in this unit test on this pr? |
Unit test for using non-existent driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -218,6 +220,19 @@ public Integer applyDirect(final String[] args) { | |||
} | |||
|
|||
final NBCLIOptions options = new NBCLIOptions(args, Mode.ParseAllOptions); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to an adjacent class, or put it into an Optional friendly call on DriverAdapterLoader or similar? We need to keep NBCLI high-level as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jshook I moved the logic to an adjacent class but it seems like some test are failing. I reviewed the code it looks fine, do you have any suggestion or should I revert the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor structural change requested, otherwise looks good.
closes #1006 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a closer look before merging this one
Description
Warn the user if the provided driver is not recognized, before op template resolution
Issue : #1006