Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Fix line splitter #62

Merged
merged 3 commits into from
Nov 22, 2017
Merged

Conversation

leafpetersen
Copy link
Contributor

This fixes dart-lang/sdk#28748 .

LineSplitter has been in an odd state for a while where it implements Converter in Dart 1.0 but not in 2.0. We've made the change in the Dart 2.0 repository to flip it entirely away from Converter. This adds a wrapper implementation that satisfies the Converter interface.

@leafpetersen
Copy link
Contributor Author

cc @tvolkert for review

cc @lrhn @kevmoo Does this look like the right fix for this issue? I'll be surprised if this doesn't come up anywhere else - should a Converter version of LineSplitter be added to dart:convert?

@leafpetersen
Copy link
Contributor Author

cc @keertip

Copy link
Contributor

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

Tiny nit...

/// A trivial conversion turning a Sink<List<String>> into a
/// Sink<String>
class _StringSinkWrapper implements Sink<String> {
Sink<List<String>> _sink;
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@keertip
Copy link
Contributor

keertip commented Nov 22, 2017

@tvolkert

@matanlurey
Copy link
Contributor

It looks like this breaks travis @leafpetersen:

https://travis-ci.org/google/file.dart/jobs/305563061

Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM.

The Travis failures look unrelated to this PR.

@leafpetersen
Copy link
Contributor Author

Yes, I think travis failures are flakes or otherwise unrelated. Initial PR passed travis: https://travis-ci.org/google/file.dart/builds/305560605 . Failing run seems to be on the next commit when I added final, but tests still pass locally.

@leafpetersen
Copy link
Contributor Author

@tvolkert Will you land and publish? I don't have write access here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

LineSplitter not assignable to Converter<String, List<String>>
5 participants