Support transformers that depend on other transformers.

Support transformers that depend on other transformers.

R=rnystrom@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=27322

[Bob Nystrom, 2013-09-06 18:27:29]

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:48: //    
transformers.
I found this hard to grok and tried rewriting it. What do you think of:

In order to determine in what order we should load transformers, we need to
know which transformers depend on which others. This is different than
normal package dependencies. First a bit of terminolgy:

* If package A is transformed by package B, we say "B transforms A".
* If code in A imports code in B or has a regular package dependency on it we
  say "A imports B".
* If B needs A's transformers to be loaded in order to load B's
  transformers, we say "A comes before B".

In particular, "comes before" is defined as follows:

* If A transforms B, A comes before B.
* If A imports B (directly or transitively) and C transforms B, C comes before
  A.

The order that transformers are loaded is determined by each package's
"comes before" relationships. We treat the packages as a directed acyclic[1]
graph where each package is a node and the "comes before" relations are the
edges. (Another way to say this is the packages form a partially ordered set.)
Then we do a topological sort on the graph.

We start loading[2] nodes with no where no other node comes before it. We then
work our way through the graph until all nodes are loaded.

[1] TODO(nweiz): support cycles in some cases.

[2] We use "loading a package" as a shorthand for loading that package's
    transformers.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:65: //
[loadPackage] first.
This is confusing to me. "incoming" and "outgoing" aren't very clear. This is
the set of packages that don't need anything to be loaded before them, right?
Clarify that.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:67:
.difference(unionAll(orderingDeps.values));
If there is a cyclic dependency, this may be an empty set. Add a TODO for this?

(You can also have cycles where this *isn't* an empty set too, I believe.)

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:79: // First,
load each package upon which [package] has an ordering dependency.
"load each package that need to come before [package]".

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:92: });
I don't mind not *supporting* cycles yet, but it would be good to at least
detect them. Otherwise this will crash, right?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:119: /// are
those packages' ordering dependencies.
How about:

"Returns a map where each key is a package and the value is the set of packages
that must be loaded before it. Packages that don't require any other package to
be loaded before it will not appear in the map."

Also, _computeOrderingDeps -> _computeOrderingRelations?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:120:
Map<String, Set<String>> _computeOrderingDeps(PackageGraph graph) {
Using "deps" here for dependencies, transformers, and ordering relations is
confusing. How about:

orderingDeps -> orderings
deps -> ordering
packageDep -> dep

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:123: var deps =
_transformerDeps(graph, package.name);
// The transformers applied to this package must be loaded first.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:125: for (var
packageDep in _transitivePackageDeps(graph, package.name)) {
// And everything those transformers import must therefore be loaded first too.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:135:
Set<String> _transitivePackageDeps(PackageGraph graph, String package) {
Move this into PackageGraph?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:150: ///
Returns the set of transformer dependencies for [package].
How about: "Returns the set of packages whose transformers are applied to
[package]."

?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:155: ///
Returns a map from each package in [graph]'s name to the asset ids of all
"package in [graph]'s name" -> "package name in graph"

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:161: for (var
id in unionAll(package.pubspec.transformers)) {
Don't bother unioning here. Just do another nested loop and add them all. Since
you're ultimately adding to a Set, dupes will be filtered out anyway.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:172: /// The
loaded transformers for each asset id.
"for" is a bit ambiguous. Took me a while to realize this was the transformers
defined in the Dart library at ID. Clarify?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:208: var path =
id.path.replaceFirst('lib/', '');
Use pathos here. This would do the wrong thing on
"joke/glib/oops/lib/transformer.dart".

Maybe we should add replace()-like methods directly in pathos?

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/runs_a_third_party_transformer_on_a_local_transformer_test.dart
(right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_third_party_transformer_on_a_local_transformer_test.dart:39:
'const TOKEN = "main.dart, foo, myapp, foo";');
It might be easier to understand what's going on here if the concatenated tokens
used grouping instead of just ",". Maybe more like:

"((main.dart, foo), (myapp, foo))"

(I think I have that right...)

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:47: /// a constant. This means it
will modify other similar transformers.
This is a brainteaser. How about:

Returns the source code for a Dart library defining a Transformer that rewrites
Dart files.

The transformer defines a constant named TOKEN whose value is [id]. When it the
transformer transforms another Dart file, it will look for a "TOKEN" constant
definition there and modify it to include *this* transformer's TOKEN value too.

If [import] is passed, it is the name of a package. The primary library of that
package will be imported here and its TOKEN value will be added to this
library's.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:67: final _tokenRegexp = new
RegExp(r'^const TOKEN = "(.*?)";\$', multiLine: true);
Capitalize "Exp" to match the type name.

[nweiz, 2013-09-09 20:43:37]

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:48: //    
transformers.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> I found this hard to grok and tried rewriting it. What do you think of:
> 
> In order to determine in what order we should load transformers, we need to
> know which transformers depend on which others. This is different than
> normal package dependencies. First a bit of terminolgy:
> 
> * If package A is transformed by package B, we say "B transforms A".
> * If code in A imports code in B or has a regular package dependency on it we
>   say "A imports B".
> * If B needs A's transformers to be loaded in order to load B's
>   transformers, we say "A comes before B".
> 
> In particular, "comes before" is defined as follows:
> 
> * If A transforms B, A comes before B.
> * If A imports B (directly or transitively) and C transforms B, C comes before
>   A.

I'm worried that these verb-based descriptions will make it more difficult to
clearly name variables and functions that deal with these dependencies. It means
there's no name for "everything that transforms a package" (or is imported by a
package, or comes before a package), which is a concept this code deals with
constantly.

I sympathize with the terminology being complex, but I think your suggestion
just shuffles around the complexity without reducing it overall. Verbs have the
advantage that they're easier to grasp immediately, but they're difficult to use
elsewhere in the code. Once you understand the nouns, it makes all the future
code and documentation much more concise and easy to understand. I think for
this code it makes sense to optimize for that.

> The order that transformers are loaded is determined by each package's
> "comes before" relationships. We treat the packages as a directed acyclic[1]
> graph where each package is a node and the "comes before" relations are the
> edges. (Another way to say this is the packages form a partially ordered set.)
> Then we do a topological sort on the graph.

Done, although modified to clarify that the code itself doesn't actually do a
topological sort.

> We start loading[2] nodes with no where no other node comes before it. We then
> work our way through the graph until all nodes are loaded.
> 
> [1] TODO(nweiz): support cycles in some cases.
> 
> [2] We use "loading a package" as a shorthand for loading that package's
>     transformers.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:65: //
[loadPackage] first.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> This is confusing to me. "incoming" and "outgoing" aren't very clear. This is
> the set of packages that don't need anything to be loaded before them, right?
> Clarify that.

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:67:
.difference(unionAll(orderingDeps.values));
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> If there is a cyclic dependency, this may be an empty set. Add a TODO for
this?
> 
> (You can also have cycles where this *isn't* an empty set too, I believe.)

There's a TODO in _computeOrderingDeps to detect and fail on cycles.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:79: // First,
load each package upon which [package] has an ordering dependency.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> "load each package that need to come before [package]".

See above.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:92: });
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> I don't mind not *supporting* cycles yet, but it would be good to at least
> detect them. Otherwise this will crash, right?

This will also be covered by the TODO in _computeOrderingDeps.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:119: /// are
those packages' ordering dependencies.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> How about:
> 
> "Returns a map where each key is a package and the value is the set of
packages
> that must be loaded before it. Packages that don't require any other package
to
> be loaded before it will not appear in the map."
> 
> Also, _computeOrderingDeps -> _computeOrderingRelations?

See above.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:120:
Map<String, Set<String>> _computeOrderingDeps(PackageGraph graph) {
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> Using "deps" here for dependencies, transformers, and ordering relations is
> confusing. How about:
> 
> orderingDeps -> orderings
> deps -> ordering
> packageDep -> dep

See above. Also, using "ordering" here to refer to an unordered set that doesn't
represent an ordering seems very confusing.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:123: var deps =
_transformerDeps(graph, package.name);
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> // The transformers applied to this package must be loaded first.

Done, using my terminology.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:125: for (var
packageDep in _transitivePackageDeps(graph, package.name)) {
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> // And everything those transformers import must therefore be loaded first
too.

That isn't quite what this code is doing, but I've added a comment.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:135:
Set<String> _transitivePackageDeps(PackageGraph graph, String package) {
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> Move this into PackageGraph?

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:150: ///
Returns the set of transformer dependencies for [package].
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> How about: "Returns the set of packages whose transformers are applied to
> [package]."
> 
> ?

See above.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:155: ///
Returns a map from each package in [graph]'s name to the asset ids of all
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> "package in [graph]'s name" -> "package name in graph"

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:161: for (var
id in unionAll(package.pubspec.transformers)) {
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> Don't bother unioning here. Just do another nested loop and add them all.
Since
> you're ultimately adding to a Set, dupes will be filtered out anyway.

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:172: /// The
loaded transformers for each asset id.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> "for" is a bit ambiguous. Took me a while to realize this was the transformers
> defined in the Dart library at ID. Clarify?

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:208: var path =
id.path.replaceFirst('lib/', '');
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> Use pathos here. This would do the wrong thing on
> "joke/glib/oops/lib/transformer.dart".
> 
> Maybe we should add replace()-like methods directly in pathos?

There should be no way that [id.path] doesn't begin with "lib/". Given that, I
think it's cleaner to just use string methods here. I've added an assert to that
effect, though.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/runs_a_third_party_transformer_on_a_local_transformer_test.dart
(right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_third_party_transformer_on_a_local_transformer_test.dart:39:
'const TOKEN = "main.dart, foo, myapp, foo";');
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> It might be easier to understand what's going on here if the concatenated
tokens
> used grouping instead of just ",". Maybe more like:
> 
> "((main.dart, foo), (myapp, foo))"
> 
> (I think I have that right...)

Good idea. Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:47: /// a constant. This means it
will modify other similar transformers.
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> This is a brainteaser. How about:
> 
> Returns the source code for a Dart library defining a Transformer that
rewrites
> Dart files.
> 
> The transformer defines a constant named TOKEN whose value is [id]. When it
the
> transformer transforms another Dart file, it will look for a "TOKEN" constant
> definition there and modify it to include *this* transformer's TOKEN value
too.
> 
> If [import] is passed, it is the name of a package. The primary library of
that
> package will be imported here and its TOKEN value will be added to this
> library's.

Done.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:67: final _tokenRegexp = new
RegExp(r'^const TOKEN = "(.*?)";\$', multiLine: true);
On 2013/09/06 18:27:29, Bob Nystrom wrote:
> Capitalize "Exp" to match the type name.

Done.

[Bob Nystrom, 2013-09-09 23:34:57]

I still would really prefer some more concrete terminology here. Added some
suggestions. Do with as you will then LGTM.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:48: //    
transformers.
On 2013/09/09 20:43:38, nweiz wrote:
> On 2013/09/06 18:27:29, Bob Nystrom wrote:
> > I found this hard to grok and tried rewriting it. What do you think of:
> > 
> > In order to determine in what order we should load transformers, we need to
> > know which transformers depend on which others. This is different than
> > normal package dependencies. First a bit of terminolgy:
> > 
> > * If package A is transformed by package B, we say "B transforms A".
> > * If code in A imports code in B or has a regular package dependency on it
we
> >   say "A imports B".
> > * If B needs A's transformers to be loaded in order to load B's
> >   transformers, we say "A comes before B".
> > 
> > In particular, "comes before" is defined as follows:
> > 
> > * If A transforms B, A comes before B.
> > * If A imports B (directly or transitively) and C transforms B, C comes
before
> >   A.
> 
> I'm worried that these verb-based descriptions will make it more difficult to
> clearly name variables and functions that deal with these dependencies. It
means
> there's no name for "everything that transforms a package" (or is imported by
a
> package, or comes before a package), which is a concept this code deals with
> constantly.
> 
> I sympathize with the terminology being complex, but I think your suggestion
> just shuffles around the complexity without reducing it overall. Verbs have
the
> advantage that they're easier to grasp immediately, but they're difficult to
use
> elsewhere in the code. Once you understand the nouns, it makes all the future
> code and documentation much more concise and easy to understand. I think for
> this code it makes sense to optimize for that.

I think I tried to catch most of the comments here and suggest wording that
worked with the verbs. I don't recall having any problems.

I definitely see the value of using nouns here. The problem is its all the same
noun. It's all dependency dependency dependency. That's already a long, loaded,
confusing noun, so reusing it in additional contexts with some adjectives just
makes it harder.

It's also problematic because the nouns don't often clearly indicate direction.
If A has an "ordering dependency" on B, does that mean it must be loaded before
or after? If A has a "transformer dependency" on B, is A a transformer that is
applied to B or vice versa?

I really did have to manually rewrite this code to change the terminology before
I could even understand what it did.

How about:

"A transforms B" -> A is a "transformer" of B, B is a "transformee" of A.
"A imports B" -> A is an "importer" of B, B is an "importee" of A.
"A comes before B" -> A is an "earlier package" than B. B is a "later package"
than A.

?

https://codereview.chromium.org/24016002/diff/8001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/8001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:43: // packages
in the topological sort order of this graph.
Nit but "the" -> "a". There are often multiple valid topological sorts for a
given poset.

[nweiz, 2013-09-10 00:47:44]

I considered changing the terminology, but I decided not to for reasons outlined
below.

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:48: //    
transformers.
On 2013/09/09 23:34:57, Bob Nystrom wrote:
> I think I tried to catch most of the comments here and suggest wording that
> worked with the verbs. I don't recall having any problems.

All of your rewrites were more verbose than the original wording. I also found
them harder to understand, because the lack of clearly identifiable terms failed
to call out the core concepts that were being discussed. The phrase "transformer
dependency" refers only to the notion that's been precisely defined above; the
phrase "transforms" doesn't.


> I definitely see the value of using nouns here. The problem is its all the
same
> noun. It's all dependency dependency dependency. That's already a long,
loaded,
> confusing noun, so reusing it in additional contexts with some adjectives just
> makes it harder.

But they are all dependencies. That word provides important context to
understanding the nature of these relations between packages (see below). I
don't understand the difficulty in thinking of "transformer dependency" as a
single term without confusing it with the more general notion of a dependency.

> It's also problematic because the nouns don't often clearly indicate
direction.
> If A has an "ordering dependency" on B, does that mean it must be loaded
before
> or after? If A has a "transformer dependency" on B, is A a transformer that is
> applied to B or vice versa?

This is clearly indicated by the use of the word "dependency". If A has a
dependency on B, that means A needs B to function. This is consistently true for
all three types of dependency referred to here.

> I really did have to manually rewrite this code to change the terminology
before
> I could even understand what it did.

I believe this, but I suspect that it's possible that the act of rewriting the
code contributed more to your understanding than the form to which you rewrote
it. This code performs complicated analysis and transformation on relations that
have subtle distinctions between them; there's going to be a baseline difficulty
of understanding regardless of the terminology used. Given that, I want the
terminology to be maximally precise.

> How about:
> 
> "A transforms B" -> A is a "transformer" of B, B is a "transformee" of A.
> "A imports B" -> A is an "importer" of B, B is an "importee" of A.
> "A comes before B" -> A is an "earlier package" than B. B is a "later package"
> than A.
> 
> ?

I really don't like the idea of overloading the word "transformer" here.
"Transformer" is a word that already has a well-defined meaning in this code, a
meaning that's completely separate from what I'm referring to as a "transformer
dependency".

"Importer"/"importee" is better, since it doesn't have an existing meaning, but
the point of package dependencies are several steps away from the fact that
they're imported by the package in question, so that makes the name a little
misleading. It's even possible (although unlikely) for a package dependency not
to be imported anywhere in the dependent package.

"Earlier package" and "later package" are similar to my terminology in that
they're an adjective modifying an already-in-use noun. Obviously I don't have a
problem with this in general, but I'm not sure what using "package" rather than
"dependency" buys us. These names also don't provide a clear name for the graph
of ordering dependencies.

https://codereview.chromium.org/24016002/diff/8001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24016002/diff/8001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:43: // packages
in the topological sort order of this graph.
On 2013/09/09 23:34:57, Bob Nystrom wrote:
> Nit but "the" -> "a". There are often multiple valid topological sorts for a
> given poset.

Done.

[nweiz, 2013-09-10 00:49:16]

Committed patchset #2 manually as r27322 (presubmit successful).