-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Clean up options and flags for package
, build
, and test
subcommands
#370
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
This cleans up the options that the new `package` and the old `build` and `test` subcommands take. Things had become ugly after implementing SE-0085.
@@ -37,11 +37,11 @@ private enum Mode: Argument, Equatable, CustomStringConvertible { | |||
|
|||
init?(argument: String, pop: () -> String?) throws { | |||
switch argument { | |||
case "--configuration", "--conf", "-c": | |||
case "--configuration", "--config": |
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'm not sure if we should drop -c
just yet, I think people and scripts are likely to rely on it.
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.
Ok, I've added it back. There is perhaps a broader conversation to be had about whether we in general support old-style single-dash/single-char short forms or just the double-dash/whole-word forms. I've looked at various tools for example and they seem all over the map about this, and not just the ancient ones either.
But we certainly don't want to break anything, so I've added it back.
Thanks for all the comments — I'm preparing another iteration of the PR. |
case generateXcodeproj(String?) | ||
case dumpPackage(String?) | ||
case generateXcodeproj | ||
case dumpPackage | ||
|
||
init?(argument: String, pop: () -> String?) throws { | ||
switch argument { | ||
case "init", "initialize": |
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.
Do we want to get rid of initialize
here? Seems wasteful.
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.
+1 to getting rid of it
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.
Gone.
I think I've addressed all the feedback. So far I haven't seen any dissent to the email I sent earlier, so I think we're converging. |
LGTM |
a supported abbreviation of -h).
order in which they're listed (internally only; the help is unaffected, since that order has been carefully chosen for user interface purposes).
LGTM |
Great, thanks for the reviews and comments. I'm merging this now. |
Clean up options and flags for `package`, `build`, and `test` subcommands
Changes for https://bugs.swift.org/browse/SR-1605
This cleans up the options that the new
package
and the oldbuild
and
test
subcommands take. Things had become ugly after implementingSE-0085.