Skip to content

Commit

Permalink
Simplify build failure output by always using NNN arguments (plural)
Browse files Browse the repository at this point in the history
When Blaze reports a compilation error, it shows the first bit of the command line used to invoke the compiler. When more than 200 characters have been shown (`APPROXIMATE_MAXIMUM_MESSAGE_LENGTH`), the rest of the command line is shown as:
```
(remaining NNN argument(s) skipped)
```

When I see the message in my console, it's normally because 100+ arguments are being skipped. So the correct form is then:
```
(remaining NNN arguments skipped)
```

I find this more readable since it avoids a nested parenthesis and it uses the correct plural form. Put differently, the `argument(s)` form is asking the reader to do the mental work of selecting between `argument` and `arguments` — which is unnecessary in the vast majority of cases.

RELNOTES: Simplify build failure output by always using `NNN arguments`.
PiperOrigin-RevId: 367620498
  • Loading branch information
mgeisler authored and copybara-github committed Apr 9, 2021
1 parent cf9f89c commit d51a88f
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,11 @@ public static String describeCommand(
for (String commandElement : commandLineElements) {
if (form == CommandDescriptionForm.ABBREVIATED
&& message.length() + commandElement.length() > APPROXIMATE_MAXIMUM_MESSAGE_LENGTH) {
message.append(
" ... (remaining " + numberRemaining + " argument(s) skipped)");
message
.append(" ... (remaining ")
.append(numberRemaining)
.append(numberRemaining == 1 ? " argument" : " arguments")
.append(" skipped)");
break;
} else {
if (numberRemaining < size) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void describeCommandError() throws Exception {
+ "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 "
+ "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 "
+ "arg27 arg28 arg29 arg30 arg31 "
+ "... (remaining 8 argument(s) skipped)");
+ "... (remaining 8 arguments skipped)");
assertThat(verboseMessage)
.isEqualTo(
"error executing command \n"
Expand All @@ -72,6 +72,28 @@ public void describeCommandError() throws Exception {
+ "Execution platform: //platform:exec");
}

@Test
public void describeCommandErrorWithSingleSkippedArgument() throws Exception {
String[] args = new String[35]; // Long enough to make us skip 1 argument below.
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
args[i] = "arg" + i;
}
Map<String, String> env = new LinkedHashMap<>();
String cwd = "/my/working/directory";
PlatformInfo executionPlatform =
PlatformInfo.builder().setLabel(Label.parseAbsoluteUnchecked("//platform:exec")).build();
String message =
CommandFailureUtils.describeCommandError(
false, Arrays.asList(args), env, cwd, executionPlatform);
assertThat(message)
.isEqualTo(
"error executing command some_command arg1 arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9"
+ " arg10 arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 arg19 arg20 arg21 arg22"
+ " arg23 arg24 arg25 arg26 arg27 arg28 arg29 arg30 arg31 arg32 arg33 ..."
+ " (remaining 1 argument skipped)");
}

@Test
public void describeCommandFailure() throws Exception {
String[] args = new String[3];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ public void longCommand() throws Exception {
assertThrows(CommandException.class, () -> new Command(args, env, directory).execute());
String message = CommandUtils.describeCommandError(false, exception.getCommand());
String verboseMessage = CommandUtils.describeCommandError(true, exception.getCommand());
assertThat(message)
.isEqualTo(
"error executing command this_command_will_not_be_found arg1 "
+ "arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 arg10 "
+ "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 "
+ "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 "
+ "arg27 arg28 arg29 arg30 "
+ "... (remaining 9 argument(s) skipped)");
assertThat(message)
.isEqualTo(
"error executing command this_command_will_not_be_found arg1 "
+ "arg2 arg3 arg4 arg5 arg6 arg7 arg8 arg9 arg10 "
+ "arg11 arg12 arg13 arg14 arg15 arg16 arg17 arg18 "
+ "arg19 arg20 arg21 arg22 arg23 arg24 arg25 arg26 "
+ "arg27 arg28 arg29 arg30 "
+ "... (remaining 9 arguments skipped)");
assertThat(verboseMessage)
.isEqualTo(
"error executing command \n"
Expand Down

0 comments on commit d51a88f

Please sign in to comment.