-
Notifications
You must be signed in to change notification settings - Fork 207
Handle subcommands #28
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
Conversation
|
||
self.diagnosticEngine = DiagnosticsEngine(handlers: [diagnosticsHandler]) | ||
|
||
if case .subcommand = try Self.invocationRunMode(forArgs: args) { | ||
diagnosticEngine.emit(.error_subcommand_in_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.
I think we should throw
here, so we don't continue trying to parse the rest of the command line.
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.
Sure thing, but then we can't check the exact error type in the tests, since Driver.Error
is internal - should it be made public?
Alternatively, we could add descriptions to the errors and match those, but I tried overriding implementing localizedDescription
and it was not called for some reason.
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!
@swift-ci please test |
@DougGregor is there anything else I should change here? |
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.
Looks great, thank you!
This tries to implement the handling of subcommands based on the original driver. I must admit I'm a little out of my depth here so I'd appreciate some guidance:
exec
?exec
the right function to use here?Process.findExecutable
the right method to use in place ofgetExecutablePath
?llvm::sys::path::stem
andbasenameWithoutExt
- namelybasenameWithoutExt
will return.txt
for/foo/.txt
, whilellvm::sys::path::stem
will return an empty string. Is this a problem? If so, should I implement something likestem
onVirtualPath
?Thank you very much for your time 🙂