-
Notifications
You must be signed in to change notification settings - Fork 305
Adopt swift-argument-parser for argument parsing #304
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
@swift-ci please test |
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.
Thanks for working on this!
I've kept the new completion server arguments as single-dash options (such as -completion-server-side-filtering), but as these weren't released in 5.3, I think it's a good opportunity for making them double-dash options for consistency, let me know what you think.
I agree, this is a good time to switch these ones to double-dash.
var clangdOptions = [String]() | ||
|
||
@Option( | ||
name: .customLong("index-store-path", withSingleDash: true), |
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 suggest accepting both single- and double-dash for this option if 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.
@natecook1000 please correct me if I'm wrong, but I don't think that accepting both is currently 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.
🤔 It looks like this is prohibited by the command validation, but doesn't necessarily need to be. Opened an issue to allow it: apple/swift-argument-parser#231
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.
@MaxDesiatov This fix is included in version 0.3.1! 👍
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.
This is great, thank you!
var indexStorePath: AbsolutePath? | ||
|
||
@Option( | ||
name: .customLong("index-db-path", withSingleDash: true), |
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 suggest accepting both single- and double-dash for this option if possible.
@swift-ci please test |
@swift-ci please test |
Would you be able to merge this please? I don't have commit access to this repository. Thanks! |
After swiftlang/swift-package-manager#2653 is merged, I think it would make sense for SourceKit-LSP to adopt the new
ArgumentParser
module as well. I've kept the new completion server arguments as single-dash options (such as-completion-server-side-filtering
), but as these weren't released in 5.3, I think it's a good opportunity for making them double-dash options for consistency, let me know what you think.CC @natecook1000