-
Notifications
You must be signed in to change notification settings - Fork 377
Respect the COLUMNS and LINES environment variables when present #596
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
Changes from all commits
3e7b87a
0ca9d1f
439666a
0349f16
2fcb02f
b0a6ec9
fe4b90f
a950fb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,8 @@ fileprivate struct Qux: ParsableArguments { | |
| fileprivate struct Quizzo: ParsableArguments { | ||
| @Option() var name: String | ||
| @Flag() var verbose = false | ||
| let count = 0 | ||
| let count: Int | ||
| init() { self.count = 0 } // silence warning about count not being decoded | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. THANK YOU
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1000 this might allow us to enable "warnings as errors"!! |
||
| } | ||
|
|
||
| extension UnparsedValuesEndToEndTests { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -799,3 +799,55 @@ extension HelpGenerationTests { | |
| """) | ||
| } | ||
| } | ||
|
|
||
| extension HelpGenerationTests { | ||
| private struct WideHelp: ParsableCommand { | ||
| @Argument(help: "54 characters of help, so as to wrap when columns < 80") | ||
| var argument: String? | ||
| } | ||
|
|
||
| func testColumnsEnvironmentOverride() throws { | ||
| #if os(Windows) || os(WASI) | ||
| throw XCTSkip("Unsupported on this platform") | ||
| #endif | ||
|
|
||
| defer { unsetenv("COLUMNS") } | ||
| unsetenv("COLUMNS") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to I'm actually wondering if this is safely testable when swift test --parallel is used...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof, that's a good point - Definitionally, it can't really be parallel-safe, in that environment variables themselves are mutable shared global state. The best I think I can do is lessen the amount of variance in the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if the best solution is to just not set the env var and skip test coverage. I know thats a gross solution, but I'd much rather have 100% reliable tests than missing test coverage (imo)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a partial workaround - I can use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: I tried this solution and it worked in multiple Swift versions on both platforms, so I pushed the code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, could we have this test be the only place that checks that the default 80 column width is used, and just specify exact widths in all the other help generation tests? If we're skipping parallel tests and Xcode tests, this will only get run in Linux CI (at least by me)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, yeah, the tests should probably be doing that anyway - in my default Terminal setup (in which I
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @natecook1000 Okay, pushed an update that makes all the tests in the same target specify the columns explicitly (parallel testing is inconsistent about tests in other cases in the same target, but tests in other targets always run in a different runner process), and force-unsets the environment variable in the other targets. |
||
| AssertHelp(.default, for: WideHelp.self, columns: nil, equals: """ | ||
| USAGE: wide-help [<argument>] | ||
|
|
||
| ARGUMENTS: | ||
| <argument> 54 characters of help, so as to wrap when columns < 80 | ||
|
|
||
| OPTIONS: | ||
| -h, --help Show help information. | ||
|
|
||
| """) | ||
|
|
||
| setenv("COLUMNS", "60", 1) | ||
| AssertHelp(.default, for: WideHelp.self, columns: nil, equals: """ | ||
| USAGE: wide-help [<argument>] | ||
|
|
||
| ARGUMENTS: | ||
| <argument> 54 characters of help, so as to | ||
| wrap when columns < 80 | ||
|
|
||
| OPTIONS: | ||
| -h, --help Show help information. | ||
|
|
||
| """) | ||
|
|
||
| setenv("COLUMNS", "79", 1) | ||
| AssertHelp(.default, for: WideHelp.self, columns: nil, equals: """ | ||
| USAGE: wide-help [<argument>] | ||
|
|
||
| ARGUMENTS: | ||
| <argument> 54 characters of help, so as to wrap when columns < | ||
| 80 | ||
|
|
||
| OPTIONS: | ||
| -h, --help Show help information. | ||
|
|
||
| """) | ||
| } | ||
| } | ||
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 appreciate the explicit
selfThere 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.
Suffice to say I wish there was an
.enableUpcomingFeature()flag called"DisallowImplicitSelf"🤣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.
+10000000