Show the dependender(s) for missing packages.

Show the dependender(s) for missing packages.

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

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

[nweiz, 2013-12-13 22:39:28]

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:322:
DependencyNotFoundException(String package, this._originalMessage,
I think it would be cleaner and more in keeping with ApplicationException to
keep the original exception and its stack trace rather than just its string
representation.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:326: final String
_originalMessage;
Instance variables and getters should be above the constructor, classes above
notwithstanding.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:327: String get
_message => "$_originalMessage\nDepended on by";
Nit: I don't think the original message is likely to be long enough to warrant a
newline before "Depended on by".

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:329: String
_describeDependency(PackageDep dep) => "";
Add a comment explaining why this is returning an empty string.

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

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:160: "Could not find package
'$package' at $url.", error, stackTrace);
Minor thing, but why are we using single quotes in these strings now? Double
quotes seem correct to me, since (when printed) they aren't contained within
another quoted passage.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:814: : super(message, innerError,
innerTrace);
Indent +4 spaces.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/test/h...
File
sdk/lib/_internal/pub/test/hosted/fail_gracefully_on_missing_package_test.dart
(right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/test/h...
sdk/lib/_internal/pub/test/hosted/fail_gracefully_on_missing_package_test.dart:21:
"\nDepended on by:\n"
All regexp strings should use r"", even if they don't have any active
characters. Otherwise it's too easy to accidentally modify them incorrectly.

[Bob Nystrom, 2013-12-16 23:25:11]

I went through and cleaned up all of the output messages I could find to not
quote package names. We still have a bunch of error messages that use single
quotes for things like YAML property names. I think those should be
double-quoted (or at least consistent), but that's a bit out of scope here.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:322:
DependencyNotFoundException(String package, this._originalMessage,
On 2013/12/13 22:39:28, nweiz wrote:
> I think it would be cleaner and more in keeping with ApplicationException to
> keep the original exception and its stack trace rather than just its string
> representation.

Done, except didn't bother with stack trace. This is a handled runtime error, so
we don't need and shouldn't capture a stack trace.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:326: final String
_originalMessage;
On 2013/12/13 22:39:28, nweiz wrote:
> Instance variables and getters should be above the constructor, classes above
> notwithstanding.

Done. Fixed other classes too.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:327: String get
_message => "$_originalMessage\nDepended on by";
On 2013/12/13 22:39:28, nweiz wrote:
> Nit: I don't think the original message is likely to be long enough to warrant
a
> newline before "Depended on by".

I originally had it on one line, but in my testing I felt like this looked
better. The 404 error, including the URL is pretty long.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:329: String
_describeDependency(PackageDep dep) => "";
On 2013/12/13 22:39:28, nweiz wrote:
> Add a comment explaining why this is returning an empty string.

Done.

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

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/hosted.dart:160: "Could not find package
'$package' at $url.", error, stackTrace);
On 2013/12/13 22:39:28, nweiz wrote:
> Minor thing, but why are we using single quotes in these strings now? Double
> quotes seem correct to me, since (when printed) they aren't contained within
> another quoted passage.

We were being inconsistent here so I picked one arbitrarily. After poking
around, we're still inconsistent. I think the cleanest answer is to not quote
package names. They'll always be a single word, so I think unquoted looks fine.

Fixed all the places I could find.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:814: : super(message, innerError,
innerTrace);
On 2013/12/13 22:39:28, nweiz wrote:
> Indent +4 spaces.

Done.

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/test/h...
File
sdk/lib/_internal/pub/test/hosted/fail_gracefully_on_missing_package_test.dart
(right):

https://codereview.chromium.org/114193010/diff/1/sdk/lib/_internal/pub/test/h...
sdk/lib/_internal/pub/test/hosted/fail_gracefully_on_missing_package_test.dart:21:
"\nDepended on by:\n"
On 2013/12/13 22:39:28, nweiz wrote:
> All regexp strings should use r"", even if they don't have any active
> characters. Otherwise it's too easy to accidentally modify them incorrectly.

Done. In this case, the lack of "r" was deliberate because of the escaped
newlines, but switched to use a multline string.

[nweiz, 2013-12-17 01:14:42]

lgtm

[Bob Nystrom, 2013-12-19 01:13:26]

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