Repository navigation
Conversation
039d59a to
2d299ca
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the process exit code validation in runCommand by replacing the checkExitCode, expectNonZeroExit, and expectedExitCode parameters with a unified ExpectedExit sealed class. The runAndGetStdout helper function is removed, and all call sites of runCommand across the codebase are updated to use the new ExpectedExit options. Additionally, corresponding unit tests are updated to verify the behavior of each ExpectedExit type. There are no review comments, and no further feedback is provided.
| if (checkExitCode && | ||
| ((result.exitCode == 0) == expectNonZeroExit || | ||
| (expectedExitCode != null && result.exitCode != expectedExitCode))) { | ||
| final String? expectedExitDescription = switch (expectedExit) { |
There was a problem hiding this comment.
expectedExitDescription is serving two purposes: 1. indicates whether exit code validation failed, and 2. Provides a single-word description used in the reported error message.
Neither of these purposes is immediately obvious from the variable's name or on a quick read of the switch expression. You have to parse a somewhat dense switch expression to figure out what is going on.
Instead of having this variable and switch expression serve these two purposes, what do you think about making ExpectedExit responsible for these two things? I'm not very familiar with modern Dart syntax and patterns, so I don't know if this is actually better. But to my Dart-inexperienced eyes it seems more readable. Something like:
sealed class ExpectedExit {
...
bool matches(int actualExitCode);
String get description;
}
final class _ExpectedExitCode extends ExpectedExit {
...
@override
bool matches(int actualExitCode) => actualExitCode == exitCode;
@override
String get description => exitCode == 0 ? 'zero' : '$exitCode';
}
final class _ExpectedNonZeroExit extends ExpectedExit {
...
@override
bool matches(int actualExitCode) => actualExitCode != 0;
@override
String get description => 'non-zero';
}
final class _ExpectedAnyExit extends ExpectedExit {
...
@override
bool matches(int actualExitCode) => true;
@override
String get description => 'any';
}and then the code here would be:
if (!expectedExit.matches(result.exitCode)) {
...
'...but expected ${expectedExit.description} exit code...'
...There was a problem hiding this comment.
Oh yeah that's a lot better, thanks. Done
andywolff
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/196479, because - The status or check suite Presubmit has failed. Please fix the issues identified (or deflake) before re-applying this label. |
andywolff
added
the
autosubmit
Fixes #196126
Replaces the overlapping exit-code flags on
runCommandwith a singleExpectedExittype, migrates the existing callers acrossdev/bots/, and removes a couple of unused helpers found during the cleanup.