Skip to content
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

Reenable testUsage() as testInvalidUsage() #8014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

plemarquand
Copy link
Contributor

@plemarquand plemarquand commented Oct 1, 2024

I believe its expected for the argument -halp to show the help string in swift-argument-parser, as if there is a -h or --help anywhere in the arguments all other inputs are disregarded and the help string is shown.

Instead use an invalid argument to drive the test and reenable it.

Some lines in this file used tabs instead of spaces, so the diff is a bit nosier than it should be.

I believe its expected for the argument `-halp` to show the help string
in swift-argument-parser, as if there is a `-h` or `--help` anywhere in
the arguments all other inputs are disregarded and the help string is
shown.

Instead use an invalid argument to drive the test and reenable it.
@plemarquand
Copy link
Contributor Author

@swift-ci test

@@ -48,10 +48,9 @@ final class PackageCommandTests: CommandsTestCase {
XCTAssertMatch(stdout, .contains("USAGE: swift package"))
}

func testUsage() async throws {
throw XCTSkip("rdar://131126477")
Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have this radar resolved then? I find it confusing that any option that starts with -h leads to this unexpected behavior. We should at least raise this on the SAP repo for them to resolve this first, especially as this is a regression from Swift Argument Parser 1.2.2.

Copy link
Contributor Author

@plemarquand plemarquand Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is works as intended, but maybe @natecook1000 can chime in. If any argument resolves to a help flag it takes precedence and shows the help string.

Swift Argument Parser respects combined flags, so -halp is equivalent to -h -a -l -p. Short flags can be combined, and short flags must also be only one character, so -halp isn’t a valid as a single flag. Only short flags can be combined, so —help-foo won’t match —help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rauhul

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that's the default, given that both swiftc and clang stick to single dash options, and thus clang -fblocks is clearly not equivalent to clang -f -b -l -o -c -k -s.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swiftc and clang also use options with -long-name so I wouldn't take them as amazing prior art. arg parser follows the pattern of ls -asl and git clean -Xdf where short names are combined.

Copy link
Contributor Author

@plemarquand plemarquand Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience with this type of combined syntax is as a POSIX convention. See 12.2, Guideline #5 in the POSIX Utility Conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxDesiatov @rauhul Any lingering concerns with this PR/behaviour?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with it

@plemarquand
Copy link
Contributor Author

@swift-ci test

1 similar comment
@plemarquand
Copy link
Contributor Author

@swift-ci test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants