Skip to content
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

[breaking change] Add lineTerminator field to Stdout #53863

Closed
brianquinlan opened this issue Oct 25, 2023 · 9 comments
Closed

[breaking change] Add lineTerminator field to Stdout #53863

brianquinlan opened this issue Oct 25, 2023 · 9 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io triaged Issue has been triaged by sub team

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Oct 25, 2023

Change Intent

To make it possible for stdout/stderr to correctly output Windows line endings ('\r\n') when write/writeln by adding a new field:

class _StdSink implements IOSink {
  ...
  /// The line ending used by [writeln] and to translate `"\n"` written by
  /// the [write], [writeln] and [writeAll] and [writeCharCode] methods.
  ///
  /// If a string containing `"\n"`s is written using one of the above
  /// "write" methods, then the embedded `"\n"`s are translated to the value
  /// set in `lineTerminator`. If the string contains Windows line endings
  /// (`"\r\n"`) then they are not translated. Carriage returns (`"\r`) are
  /// not translated.
  ///
  /// If `lineTerminator` is `"\n"` then the written strings are not modified.
  //
  /// Setting `lineTerminator` to [Platform.lineTerminator] will result in
  /// "write" methods outputting the line endings for the platform:
  ///
  /// ```dart
  /// stdout.lineTerminator = Platform.lineTerminator;
  /// stderr.lineTerminator = Platform.lineTerminator;
  /// ```
  ///
  /// The value of `lineTerminator` has no effect on byte-oriented methods
  /// such as [add].
  ///
  /// The value of `lineTerminator` does not effect the output of the [print]
  /// function.
  /// 
  /// Throws [ArgumentError] if set to a value other than `"\n"` or `"\r\n"`.
  set lineTerminator(String eol);
  ...
}

The default value of lineTerminator will be "\n" so the output will not change for existing Dart code.

Justification

To write correct terminal applications on Windows, developers currently have to write awkward code like:

stdout.write("Hello World" + Platform.lineTerminator);

Impact

Any class that implements Stdout without using Mock/Mock or overriding noSuchMethod will break.

There are 100s of files on github that implement Stdout but most them that I looked at use Fake/Mock or override noSuchMethod.

A few do not:
https://github.com/onepub-dev/minion/blob/57cda065a6fc489839d0cbf80d15f6d08432e614/lib/src/stdout.dart#L7
https://github.com/tvolkert/universal_io/blob/02f99955f2dfbc94269ddfa8d326a1bbe7787e8e/lib/src/driver_base/base_std_out.dart#L21
https://github.com/filiph/linkcheck/blob/8ab5f5b516701f98c18cb2f16a73e5f93ebf7f12/test/e2e_test.dart#L218

Mitigation

Developers will have to add:

+   String lineEnding;

to their Stdout implementations.

Change Timeline

N/A

Associated CLs

@brianquinlan brianquinlan added the breaking-change-request This tracks requests for feedback on breaking changes label Oct 25, 2023
@brianquinlan brianquinlan changed the title [breaking change] <title> [breaking change] Add lineTerminator field to Stdout Oct 25, 2023
@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io labels Oct 26, 2023
@itsjustkevin
Copy link
Contributor

Breaking change request incoming @grouma @Hixie @vsmenon

@vsmenon
Copy link
Member

vsmenon commented Nov 6, 2023

Sounds reasonable, but ... should we consider changing the Win default? @a-siva @mraleph @tvolkert

@grouma
Copy link
Member

grouma commented Nov 6, 2023

SGTM. No impact to ACX.

@tvolkert
Copy link
Contributor

tvolkert commented Nov 7, 2023

FYI, https://github.com/tvolkert/universal_io was a clone of another repo -- I just deleted my clone.

@mraleph
Copy link
Member

mraleph commented Nov 7, 2023

Sounds reasonable, but ... should we consider changing the Win default? @a-siva @mraleph @tvolkert

I think that could be a reasonable breaking change, though it might lead to some subtle breakages when output contents suddenly change, e.g. if people produce files by redirecting output then contents of these files will change.

@Hixie
Copy link
Contributor

Hixie commented Nov 9, 2023

no objection from me

@a-siva a-siva added the triaged Issue has been triaged by sub team label Nov 16, 2023
@brianquinlan
Copy link
Contributor Author

@vsmenon

Sounds reasonable, but ... should we consider changing the Win default? @a-siva @mraleph @tvolkert

I tried that initially but it is too breaking - there are a lot of CLI projects with tests that assert their output. If the default output changes to "\r\n" on Windows then their Windows tests would break.

@vsmenon
Copy link
Member

vsmenon commented Dec 3, 2023

lgtm

@brianquinlan
Copy link
Contributor Author

Fixed in 770f44d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

10 participants