google / process.dart

BSD 3-Clause "New" or "Revised" License
52 stars 26 forks source link

ProcessManager.run signature is inconsistent with LocalProcessManager.run #56

Closed jamesderlin closed 3 years ago

jamesderlin commented 3 years ago

ProcessManager has:

  Future<ProcessResult> run(
    List<dynamic> command, {
    String workingDirectory,
    Map<String, String> environment,
    bool includeParentEnvironment = true,
    bool runInShell = false,
    Encoding stdoutEncoding = systemEncoding,
    Encoding stderrEncoding = systemEncoding,
  });

LocalProcessManager has:

  @override
  Future<ProcessResult> run(
    covariant List<Object> command, {
    String? workingDirectory,
    Map<String, String>? environment,
    bool includeParentEnvironment = true,
    bool runInShell = false,
    Encoding stdoutEncoding = systemEncoding,
    Encoding stderrEncoding = systemEncoding,
  }) {

Is the difference in nullability for the workingDirectory and environment arguments intended? While legal, it's confusing, and I don't see any advantage to requiring non-null arguments in the base class.

Same for ProcessManager.start and LocalProcessManager.start.

jamesderlin commented 3 years ago

This also makes it harder to migrate code that uses dart:io's Process.run/Process.start (which also expect nullable workingDirectory and environment) to using ProcessManager: calling code would need to explicitly pass Platform.environment and the current working directory to get the same behavior as passing null directory to dart:io's Process functions.

cc @jonahwilliams