Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(247)

Issue 22396004: Make observable transform a barback transform. (Closed)

Created:
6 years, 11 months ago by Siggi Cherem (dart-lang)
Modified:
6 years, 11 months ago
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : changes alone (patchset 1 has the existing code, in the new location) #

Total comments: 58

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : review comments #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 31

Patch Set 9 : addressing john's comments #

Total comments: 4

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+942 lines, -3 lines) Patch
M pkg/barback/lib/barback.dart View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M pkg/barback/lib/src/transform.dart View 1 2 3 4 5 6 7 3 chunks +7 lines, -0 lines 0 comments Download
A pkg/barback/lib/src/transform_logger.dart View 1 2 3 4 5 6 7 1 chunk +40 lines, -0 lines 0 comments Download
M pkg/barback/pubspec.yaml View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A pkg/observe/lib/transform.dart View 1 2 3 4 5 6 7 8 1 chunk +338 lines, -0 lines 0 comments Download
M pkg/observe/pubspec.yaml View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
A pkg/observe/test/transform_test.dart View 1 2 3 4 5 6 7 8 1 chunk +121 lines, -0 lines 0 comments Download
M pkg/pkg.status View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M pkg/source_maps/lib/printer.dart View 1 2 3 4 5 6 7 8 2 chunks +158 lines, -2 lines 0 comments Download
A pkg/source_maps/lib/refactor.dart View 1 2 3 4 5 6 7 8 9 1 chunk +131 lines, -0 lines 0 comments Download
M pkg/source_maps/lib/source_maps.dart View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M pkg/source_maps/test/printer_test.dart View 1 2 3 4 5 6 7 8 1 chunk +40 lines, -0 lines 0 comments Download
A pkg/source_maps/test/refactor_test.dart View 1 2 3 4 5 6 7 8 1 chunk +96 lines, -0 lines 0 comments Download
M pkg/source_maps/test/run.dart View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
Siggi Cherem (dart-lang)
The change is smaller than it looks. I moved some code from the polymer repo ...
6 years, 11 months ago (2013-08-08 22:29:03 UTC) #1
Bob Nystrom
Adding Nathan. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart#newcode12 pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; Maybe "Logger"? ...
6 years, 11 months ago (2013-08-08 22:47:09 UTC) #2
Siggi Cherem (dart-lang)
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart#newcode12 pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On 2013/08/08 22:47:09, Bob ...
6 years, 11 months ago (2013-08-08 23:10:41 UTC) #3
Siggi Cherem (dart-lang)
oops missed to reply to one comment. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.dart#newcode36 pkg/observe/lib/transform.dart:36: // transform. ...
6 years, 11 months ago (2013-08-08 23:14:02 UTC) #4
nweiz
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart#newcode12 pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On 2013/08/08 23:10:41, Siggi ...
6 years, 11 months ago (2013-08-08 23:34:57 UTC) #5
Siggi Cherem (dart-lang)
thanks Nathan, PTAL https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart#newcode12 pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On ...
6 years, 11 months ago (2013-08-09 00:29:54 UTC) #6
Siggi Cherem (dart-lang)
https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.dart#newcode47 pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) ...
6 years, 11 months ago (2013-08-09 00:40:24 UTC) #7
nweiz
barback changes lgtm, with a couple more suggestions https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart#newcode74 pkg/barback/lib/src/transform.dart:74: /// ...
6 years, 11 months ago (2013-08-09 00:53:48 UTC) #8
Siggi Cherem (dart-lang)
thanks! https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart#newcode74 pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and ...
6 years, 11 months ago (2013-08-09 01:17:30 UTC) #9
Jennifer Messerly
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart#newcode8 pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' show Span; On 2013/08/09 00:29:54, Siggi Cherem ...
6 years, 11 months ago (2013-08-09 05:44:03 UTC) #10
Bob Nystrom
Some more comments, but barback changes LGTM. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transform.dart#newcode8 pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' ...
6 years, 11 months ago (2013-08-09 15:42:37 UTC) #11
nweiz
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart#newcode32 pkg/observe/lib/transform.dart:32: // Note: technically we should parse the file to ...
6 years, 11 months ago (2013-08-09 19:41:26 UTC) #12
Siggi Cherem (dart-lang)
Thanks guys. Replied to a few comments. Main thing left to review are the changes ...
6 years, 11 months ago (2013-08-09 22:09:09 UTC) #13
Jennifer Messerly
lgtm https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refactor.dart File pkg/source_maps/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refactor.dart#newcode103 pkg/source_maps/lib/refactor.dart:103: /** triple slash?
6 years, 11 months ago (2013-08-09 22:14:47 UTC) #14
Siggi Cherem (dart-lang)
thx! https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refactor.dart File pkg/source_maps/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refactor.dart#newcode103 pkg/source_maps/lib/refactor.dart:103: /** On 2013/08/09 22:14:47, John Messerly wrote: > ...
6 years, 11 months ago (2013-08-09 22:19:51 UTC) #15
nweiz
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart#newcode17 pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: ...
6 years, 11 months ago (2013-08-09 22:21:27 UTC) #16
Siggi Cherem (dart-lang)
Committed patchset #10 manually as r25985 (presubmit successful).
6 years, 11 months ago (2013-08-09 22:29:48 UTC) #17
Siggi Cherem (dart-lang)
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart#newcode17 pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:21:28, nweiz wrote: > On ...
6 years, 11 months ago (2013-08-09 22:38:33 UTC) #18
Jennifer Messerly
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform.dart#newcode17 pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:38:33, Siggi Cherem (dart-lang) wrote: ...
6 years, 11 months ago (2013-08-09 22:46:50 UTC) #19
nweiz
6 years, 11 months ago (2013-08-09 22:53:24 UTC) #20
Message was sent while issue was closed.
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform...
File pkg/observe/lib/transform.dart (right):

https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform...
pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart';
On 2013/08/09 22:46:50, John Messerly wrote:
> On 2013/08/09 22:38:33, Siggi Cherem (dart-lang) wrote:
> > On 2013/08/09 22:21:28, nweiz wrote:
> > > On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote:
> > > > On 2013/08/09 15:42:38, Bob Nystrom wrote:
> > > > > On 2013/08/09 05:44:03, John Messerly wrote:
> > > > > > sanity check: barback doesn't depend on "dart:io" right?
> > > > > 
> > > > > Yes, barback does. That means this library in observe is coupled to
> > dart:io,
> > > > but
> > > > > other libraries in here that don't import transform.dart aren't. Is
that
> > OK?
> > > > 
> > > > For now yes, but I think eventually it would be nice to decouple barback
a
> > > bit.
> > > > At least so that if I implement a transform I don't have to depend on
it.
> > > 
> > > I don't think it makes sense either for barback not to depend on dart:io
> > > (file-based Assets are fundamentally tied to the filesystem, and we may
put
> > > caching support in barback at some point). It's easy for a package to have
a
> > > transform that depends on dart:io without having the rest of the package's
> > > libraries do so, so I think that's likely to be what we'll do in the
future.
> > 
> > I think it would be nice to be able to run transfomers also in other
contexts
> > (e.g. in a browser's web worker, for example).
> > 
> > It's totally ok for barback to depend on dart:io, but I would be nice if it
> > doesn't push this dependency on transfomers too. I think we can achieve that
> by
> > simply splitting the Asset API (so the interface is separate from how to
> create
> > file assets). 
> 
> +1 siggi

Transformer is a class that wraps code that can do some sort of conceptual
transformation in a form that barback can understand. If you want to use that
transformation elsewhere, it's easy to factor out a library that exposes a
programmatic API for it and have the barback Transformer call out to that
library. I don't think it makes sense to change the barback API so it can be
called in a non-barback context.

Powered by Google App Engine
This is Rietveld 408576698