Skip to content

Use Stdout.terminalColumns for line length #716

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

Merged
merged 4 commits into from
Nov 2, 2017
Merged

Use Stdout.terminalColumns for line length #716

merged 4 commits into from
Nov 2, 2017

Conversation

nex3
Copy link
Member

@nex3 nex3 commented Nov 1, 2017

Closes #86

@nex3 nex3 requested a review from grouma November 1, 2017 22:28
@@ -18,6 +18,23 @@ const _newline = 0xA;
/// The ASCII code for a carriage return character.
const _carriageReturn = 0xD;

/// The default line length for output when there isn't a terminal attached to
/// stdout.
const _defaultLineLength = 200;
Copy link
Member

Choose a reason for hiding this comment

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

I think the fallback should get called out in the commit message.

Why was 200 chosen instead of the old value of 100?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the fallback should get called out in the commit message.

Done.

Why was 200 chosen instead of the old value of 100?

100 was chosen to approximate the average terminal width. Now that we detect the actual width for real terminals, I want to increase the default because it's more likely to come into play in non-width-constrained environments.

final int lineLength = () {
try {
return stdout.terminalColumns;
} on UnsupportedError catch (_) {
Copy link
Member

Choose a reason for hiding this comment

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

[nit] omit the catch if we are ignoring the caught value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -103,6 +120,36 @@ Stream<List<int>> sanitizeForWindows(Stream<List<int>> input) {
});
}

/// Wraps [text] so that it fits within [lineLength], which defaults to
/// [lineLength].
Copy link
Member

Choose a reason for hiding this comment

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

which defaults to [_defaultLineLength]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, it doesn't default at all anymore. Changed.

/// [lineLength].
///
/// This preserves existing newlines and doesn't consider terminal color escapes
/// part of a word's length.
Copy link
Member

Choose a reason for hiding this comment

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

perhaps call out in the doc comment that words are only split by space and not other whitespace characters

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Fall back on a 200-character default, which is less likely to obscure
test names when printing to a non-terminal destination.

Closes #86
@nex3 nex3 merged commit 28dfb44 into master Nov 2, 2017
@nex3 nex3 deleted the line-length branch November 2, 2017 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants