-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[tool] Guard more write/writeln calls on Process.stdin
#151146
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
[tool] Guard more write/writeln calls on Process.stdin
#151146
Conversation
d4fc3d6 to
7507cb4
Compare
write/writeln calls on Process.stdin
87427c1 to
ac15c6b
Compare
9a76fbd to
237448f
Compare
This reverts commit 5ab8a20.
237448f to
b4bcbd3
Compare
2e4f844 to
3da580c
Compare
write/writeln calls on Process.stdinwrite/writeln calls on Process.stdin
| ); | ||
| } | ||
|
|
||
| static Future<void> writeToStdinUnsafe({ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
onErrorcallback parameter--ifonErroris null then we unconditionally callcompleter.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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
christopherfujino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…#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
…#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
Contributes to fixing #137184.
This PR guards write calls in non-test files. This PR excludes
awaittoDefaultResidentCompiler._recompilefails test(s) in test/general.shard/devfs_test.dart #151255Pre-launch Checklist
///).