Skip to content

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Jul 1, 2024

Contributes to fixing #137184.

This PR guards write calls in non-test files. This PR excludes

Pre-launch Checklist

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 1, 2024
@andrewkolos andrewkolos force-pushed the guard-iosink-writeln-calls branch 3 times, most recently from d4fc3d6 to 7507cb4 Compare July 3, 2024 04:52
@andrewkolos andrewkolos changed the title [tool] guard process.stdin.write call to sdkmanager [tool] Guard all calls write/writeln calls on Process.stdin Jul 3, 2024
@andrewkolos andrewkolos force-pushed the guard-iosink-writeln-calls branch from 87427c1 to ac15c6b Compare July 3, 2024 06:11
@andrewkolos andrewkolos force-pushed the guard-iosink-writeln-calls branch 3 times, most recently from 9a76fbd to 237448f Compare July 23, 2024 00:29
@andrewkolos andrewkolos force-pushed the guard-iosink-writeln-calls branch from 237448f to b4bcbd3 Compare July 23, 2024 05:29
@andrewkolos andrewkolos force-pushed the guard-iosink-writeln-calls branch from 2e4f844 to 3da580c Compare July 23, 2024 17:38
@andrewkolos andrewkolos changed the title [tool] Guard all calls write/writeln calls on Process.stdin [tool] Guard more write/writeln calls on Process.stdin Jul 23, 2024
@andrewkolos andrewkolos marked this pull request as ready for review July 23, 2024 17:43
);
}

static Future<void> writeToStdinUnsafe({
Copy link
Contributor

Choose a reason for hiding this comment

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

What about instead of having four of these public functions, we just have one, with a boolean parameter (that defaults to true) for whether to append newline, and then a nullable onError callback parameter--if onError is null then we unconditionally call completer.completeError() with caught errors?

My concern with four functions is that contributors will cargo cult the wrong one. There's a lot of cognitive complexity here understanding the differences. In a code review also, it's difficult to parse the differences.

Copy link
Contributor Author

@andrewkolos andrewkolos Jul 23, 2024

Choose a reason for hiding this comment

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

with a boolean parameter (that defaults to true)

If you were to rewrite the IOSink API, would you remove writeln in favor of adding a boolean parameter to write with a default? If not, why should the ProcessUtils counterpart not exist as its own function?

and then a nullable onError callback parameter--if onError is null then we unconditionally call completer.completeError() with caught errors?

is

ProcessUtils.writeToStdinUnsafe(stdin: myProcess.stdin, line: 'foo');

less readable than

ProcessUtils.writeToStdinGuarded(
  stdin: myProcess.stdin,
  line: 'foo',
  onError: null,
);

?

My concern with four functions is that contributors will cargo cult the wrong one.

How are separate named functions more cargo-cultable than fewer ones with boolean parameters (especially when one or more of those boolean parameters are optional)? Looking at the two examples I wrote earlier, is the idea that onError: null is easier to notice and think about than looking at the function name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a code review also, it's difficult to parse the differences.

How so? Is this something I can address with doc comments? If the diff is difficult to parse, I'd recommend just reading the code directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 28, 2024
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…#151146)

Contributes to fixing flutter#137184.

This PR guards write calls in non-test files. This PR excludes
* packages/flutter_tools/lib/src/dart/analysis.dart due to a test timeout I would like to figure out in a separate PR and
* packages/flutter_tools/lib/src/compile.dart due to flutter#151255
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…#151146)

Contributes to fixing flutter#137184.

This PR guards write calls in non-test files. This PR excludes
* packages/flutter_tools/lib/src/dart/analysis.dart due to a test timeout I would like to figure out in a separate PR and
* packages/flutter_tools/lib/src/compile.dart due to flutter#151255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants