Respect the COLUMNS and LINES environment variables when present#596
Conversation
… determining screen size.
…alue, and clean up the code a little in the process.
|
@swift-ci please test |
| /// The current terminal size, or the default if the width is unavailable. | ||
| static var terminalWidth: Int { | ||
| terminalSize().width | ||
| self.terminalSize().width |
There was a problem hiding this comment.
I appreciate the explicit self
There was a problem hiding this comment.
Suffice to say I wish there was an .enableUpcomingFeature() flag called "DisallowImplicitSelf" 🤣
natecook1000
left a comment
natecook1000
left a comment
There was a problem hiding this comment.
This looks great! Any ideas about how to test this? Could we set COLUMNS for some/one of the help generation tests to verify that it gets picked up as the default?
| @Flag() var verbose = false | ||
| let count = 0 | ||
| let count: Int | ||
| init() { self.count = 0 } // silence warning about count not being decoded |
There was a problem hiding this comment.
+1000 this might allow us to enable "warnings as errors"!!
Test added! |
…ady set in the environment
|
@swift-ci Please test |
| throw XCTSkip("Unsupported on this platform") | ||
| #endif | ||
|
|
||
| unsetenv("COLUMNS") |
There was a problem hiding this comment.
Do we need to defer { unsetenv("COLUMNS") } to avoid this test colliding with other tests which assert on help screens?
I'm actually wondering if this is safely testable when swift test --parallel is used...
There was a problem hiding this comment.
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 COLUMNS value (such as only setting it to 80 and 81, keeping it as close as possible to the default).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I have a partial workaround - I can use CommandLine.arguments to detect when parallel testing is in effect (the commandline signatures are unique on both macOS and Linux), and use that to skip the test. The caveat is that it only works for swift test; I'd have to skip it unconditionally in Xcode.
There was a problem hiding this comment.
Update: I tried this solution and it worked in multiple Swift versions on both platforms, so I pushed the code.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Hm, yeah, the tests should probably be doing that anyway - in my default Terminal setup (in which I export COLUMNS in my ~/.bash_profile) the tests already spuriously fail even on main.
There was a problem hiding this comment.
@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.
|
@swift-ci Please test |
3 participants
As documented in both the Linux and BSD versions of the
environ(7)manpage, many console utilities respect the presence of theCOLUMNSandLINESenvironment variables, treating them as overrides (or, less commonly, fallbacks) of the terminal's reported size (if any). This adds that same behavior to ArgumentParser's screen size logic, affecting the wrapping of the usage generator's output.Note: These changes follow the semantics of
ls(1), whereCOLUMNSandLINESindividually override the values reported by a terminal when they are present. Setting only one of the two variables has no effect on the other. It is also worth noting that at the present time, overriding the screen height (i.e.LINES) has no impact whatsoever on ArgumentParser's behavior.The new logic only takes effect on non-Windows/WASI platforms.
Checklist