Make the override warnings prettier.

Make the override warnings prettier.

(Also tweak the download message.)

BUG=https://code.google.com/p/dart/issues/detail?id=15181
R=nweiz@google.com

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

[nweiz, 2013-11-26 21:14:46]

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: }
Perhaps we should encapsulate this stringifying logic in [PackageDep.toString].

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:241: /// [containingPath] references
the containing directory of the root package.
"references" -> "is"

Also mention the default value.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:243: return description;
description.toString()

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:120: sourcePath =
path.relative(description['path'], from: containingPath);
I think this should be relative to the pwd, not [containingPath]. Most of the
time the two will be the same, but if pub is run from a subdirectory of the
package I think it'll be clearer if the relative path is pointing to a path
that's resolvable from their current location. Similarly, I think it should
relativize the path even if it's absolute in the pubspec.

The purpose of this output isn't to convey what's in the pubspec, it's to convey
what the dependency is.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/test/de...
File sdk/lib/_internal/pub/test/dependency_override_test.dart (left):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/test/de...
sdk/lib/_internal/pub/test/dependency_override_test.dart:110: """);
We should also test this with a path dependency override.

[Bob Nystrom, 2013-11-26 22:29:37]

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: }
On 2013/11/26 21:14:46, nweiz wrote:
> Perhaps we should encapsulate this stringifying logic in
[PackageDep.toString].

The problem is it needs to know if a source is the default source, which
requires access to the SourceRegistry. We could hardcode an assumption about
"hosted", but that feels gross.

I thought about hoisting it out to a separate function, but it seemed premature
since nothing else is using it yet. I am interested in doing a pass over the
code where we output things and making it more consistent, but I didn't want to
make this patch too sweeping.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:241: /// [containingPath] references
the containing directory of the root package.
On 2013/11/26 21:14:46, nweiz wrote:
> "references" -> "is"
> 
> Also mention the default value.

Done.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:243: return description;
On 2013/11/26 21:14:46, nweiz wrote:
> description.toString()

Done.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:120: sourcePath =
path.relative(description['path'], from: containingPath);
On 2013/11/26 21:14:46, nweiz wrote:
> I think this should be relative to the pwd, not [containingPath]. Most of the
> time the two will be the same, but if pub is run from a subdirectory of the
> package I think it'll be clearer if the relative path is pointing to a path
> that's resolvable from their current location. Similarly, I think it should
> relativize the path even if it's absolute in the pubspec.

I tried that, but it feels and looks wrong to me. I understand what you're
getting at, but I think it's clearest to show the path that the user specified
in their pubspec. I think they'll understand it's relative to the pubspec
because that's what it was when they authored it.

Showing a different calculated path is confusing because they may wonder where
that strange relative path came from, and it isn't obvious that it's just the
output trying to be friendly.

[nweiz, 2013-11-27 01:11:39]

Did you miss my test comment?

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:120: sourcePath =
path.relative(description['path'], from: containingPath);
On 2013/11/26 22:29:38, Bob Nystrom wrote:
> On 2013/11/26 21:14:46, nweiz wrote:
> > I think this should be relative to the pwd, not [containingPath]. Most of
the
> > time the two will be the same, but if pub is run from a subdirectory of the
> > package I think it'll be clearer if the relative path is pointing to a path
> > that's resolvable from their current location. Similarly, I think it should
> > relativize the path even if it's absolute in the pubspec.
> 
> I tried that, but it feels and looks wrong to me. I understand what you're
> getting at, but I think it's clearest to show the path that the user specified
> in their pubspec. I think they'll understand it's relative to the pubspec
> because that's what it was when they authored it.
> 
> Showing a different calculated path is confusing because they may wonder where
> that strange relative path came from, and it isn't obvious that it's just the
> output trying to be friendly.

How about this: if [containingPath] isn't [path.current], we add "(from
${path.relative(containingPath)})" to the end of [sourcePath]?

[Bob Nystrom, 2013-12-03 19:10:55]

Sorry, I did miss the test comment. Fixed.

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/84223002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:120: sourcePath =
path.relative(description['path'], from: containingPath);
On 2013/11/27 01:11:39, nweiz wrote:
> On 2013/11/26 22:29:38, Bob Nystrom wrote:
> > On 2013/11/26 21:14:46, nweiz wrote:
> > > I think this should be relative to the pwd, not [containingPath]. Most of
> the
> > > time the two will be the same, but if pub is run from a subdirectory of
the
> > > package I think it'll be clearer if the relative path is pointing to a
path
> > > that's resolvable from their current location. Similarly, I think it
should
> > > relativize the path even if it's absolute in the pubspec.
> > 
> > I tried that, but it feels and looks wrong to me. I understand what you're
> > getting at, but I think it's clearest to show the path that the user
specified
> > in their pubspec. I think they'll understand it's relative to the pubspec
> > because that's what it was when they authored it.
> > 
> > Showing a different calculated path is confusing because they may wonder
where
> > that strange relative path came from, and it isn't obvious that it's just
the
> > output trying to be friendly.
> 
> How about this: if [containingPath] isn't [path.current], we add "(from
> ${path.relative(containingPath)})" to the end of [sourcePath]?

The string returned by this is already used in a large string that mentions the
source name, so that would result in something like:

- foo (from path ../blah (from blah))

I really think just showing the pubspec relative path is the cleanest thing to
do here. Given that we can't even run this code without the cwd being the same
as the pubspec directory, I don't think it's worth doing anything different
here.

[nweiz, 2013-12-03 21:03:49]

lgtm

[Bob Nystrom, 2013-12-03 21:05:35]

Committed patchset #3 manually as r30824 (presubmit successful).