Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12755024:
add cache list command to pub (Closed)
|
Created:
7 years, 9 months ago by keertip Modified:
7 years, 9 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionadd cache list command to pub
Committed: https://code.google.com/p/dart/source/detail?r=19977
Patch Set 1 #
Total comments: 67
Patch Set 2 : #Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 13 (0 generated)
This looks excellent, thanks for doing this! https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart File utils/pub/command_cache.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file 2013 https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:9: import 'dart:json' as json; Can you add a blank line between the "dart:" and other imports? We separate out the "dart:", "pkg", and relative import sections. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:17: Remove this blank line. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:18: String get description => "Works on the pub cache."; "Inspect the system cache." https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:19: String get usage => 'pub cache {list}'; 'pub cache list' https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:23: if (commandOptions.rest.isEmpty) { .rest.length != 1 https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:24: log.error('No cache command given.'); 'The cache command expects one argument.' https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:30: if (!['list'].contains(command)) { if (commandOptions.rest[0] != 'list') https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:38: for (var p in packages){ p -> package https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:43: } Add a TODO to support non-JSON format and check for a --format flag. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:44: log.message(' ${json.stringify({'pubcache': packagesObj})}'); Instead of 'pubcache', how about 'packages'? Also, remove the interpolation: log.message(json.stringify(...)); https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart File utils/pub/git_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart#new... utils/pub/git_source.dart:120: return new Future.immediate([]); Throw UnimplementedError(). https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:121: /// Returns a list of packages in the default cache Remove this comment. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:123: var url = _defaultUrl.replaceAll(new RegExp(r"^https?://"), ""); Can you pull this out into a separate function and call it from here and line 96? https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:124: return listDir(path.join(systemCacheRoot,url)).then((entries){ Space after "," and between ")" and "{". https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:127: var name = path.basename(entry).replaceAll(new RegExp(r"-(.*)"),""); How about: path.basename(entry).split('-')[0]; https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:131: return list; return entries.map((entry) { var name = ... return new Package.load... }); https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:133: throw 'Could not list packages in pub cache ${ex}'; Don't bother catching this. https://codereview.chromium.org/12755024/diff/1/utils/pub/pub.dart File utils/pub/pub.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/pub.dart#newcode45 utils/pub/pub.dart:45: 'cache': new CacheCommand() In alphabetical order please. :) https://codereview.chromium.org/12755024/diff/1/utils/pub/source.dart File utils/pub/source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/source.dart#newcode219 utils/pub/source.dart:219: /// Returns a list of [Package] that have been cached in in Pub's global cache Returns the [Package]s that have been installed in the system cache. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:17: Remove this empty line. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:18: servePackages([package("foo", "1.2.3")]); Since you're actually creating the cache, you can simplify this a bunch. Remove this line... https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:23: async(port.then((p) => dir('pub.dartlang.org', [ ...and async(port.then((p) => (since you don't use p) https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:27: ]) Let's add another package here too to make sure it lists them all. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:33: appDir([dependency("foo", "1.2.3")]).scheduleCreate(); ...and this... https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:36: output: new RegExp("Dependencies installed!\$")); ..and this... https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:38: cacheDir({"foo": "1.2.3"}).scheduleValidate(); ..and this... https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:38: cacheDir({"foo": "1.2.3"}).scheduleValidate(); ...and this... https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:41: new RegExp(r'{"pubcache":{"foo":{"version":"1.2.3","location":"[^"]+foo-1.2.3"}}}$')); Instead of a RegExp, it's probably simpler here to just validate against a string literal: output: '{"pubcache...\n' https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:43: Add tests that: 1. It exits with the right error message if you run "pub cache" 2. It exits with the right error message if you run "pub cache foo" 3. It prints an empty map if there are no cached packages.
https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart File utils/pub/command_cache.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:36: return cache.sources.defaultSource.getCachedPackages().then((packages){ Add a TODO here to add a flag to list packages from non-default sources. https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart File utils/pub/git_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart#new... utils/pub/git_source.dart:120: return new Future.immediate([]); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Throw UnimplementedError(). I'm not sure throwing UnimplementedError is necessary, since the parent class will already throw. I would add a TODO to implement this, though. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:127: var name = path.basename(entry).replaceAll(new RegExp(r"-(.*)"),""); On 2013/03/12 22:12:03, Bob Nystrom wrote: > How about: > > path.basename(entry).split('-')[0]; Both of these are dangerous if a package has a hyphen in its name. If you pass null here, it should detect the name from the pubspec, which is safer. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:41: new RegExp(r'{"pubcache":{"foo":{"version":"1.2.3","location":"[^"]+foo-1.2.3"}}}$')); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Instead of a RegExp, it's probably simpler here to just validate against a > string literal: > > output: '{"pubcache...\n' I think a RegExp is right... the location can vary. The periods and curly braces should be escaped, though.
https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart File utils/pub/command_cache.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2013/03/12 22:12:03, Bob Nystrom wrote: > 2013 Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:9: import 'dart:json' as json; On 2013/03/12 22:12:03, Bob Nystrom wrote: > Can you add a blank line between the "dart:" and other imports? We separate out > the "dart:", "pkg", and relative import sections. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:17: On 2013/03/12 22:12:03, Bob Nystrom wrote: > Remove this blank line. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:18: String get description => "Works on the pub cache."; On 2013/03/12 22:12:03, Bob Nystrom wrote: > "Inspect the system cache." Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:19: String get usage => 'pub cache {list}'; On 2013/03/12 22:12:03, Bob Nystrom wrote: > 'pub cache list' Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:23: if (commandOptions.rest.isEmpty) { On 2013/03/12 22:12:03, Bob Nystrom wrote: > .rest.length != 1 Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:24: log.error('No cache command given.'); On 2013/03/12 22:12:03, Bob Nystrom wrote: > 'The cache command expects one argument.' Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:30: if (!['list'].contains(command)) { On 2013/03/12 22:12:03, Bob Nystrom wrote: > if (commandOptions.rest[0] != 'list') Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:36: return cache.sources.defaultSource.getCachedPackages().then((packages){ On 2013/03/12 22:58:48, nweiz wrote: > Add a TODO here to add a flag to list packages from non-default sources. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:38: for (var p in packages){ On 2013/03/12 22:12:03, Bob Nystrom wrote: > p -> package Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:43: } On 2013/03/12 22:12:03, Bob Nystrom wrote: > Add a TODO to support non-JSON format and check for a --format flag. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/command_cache.dart#... utils/pub/command_cache.dart:44: log.message(' ${json.stringify({'pubcache': packagesObj})}'); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Instead of 'pubcache', how about 'packages'? > > Also, remove the interpolation: > > log.message(json.stringify(...)); Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart File utils/pub/git_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/git_source.dart#new... utils/pub/git_source.dart:120: return new Future.immediate([]); On 2013/03/12 22:58:48, nweiz wrote: > On 2013/03/12 22:12:03, Bob Nystrom wrote: > > Throw UnimplementedError(). > > I'm not sure throwing UnimplementedError is necessary, since the parent class > will already throw. I would add a TODO to implement this, though. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:121: /// Returns a list of packages in the default cache On 2013/03/12 22:12:03, Bob Nystrom wrote: > Remove this comment. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:123: var url = _defaultUrl.replaceAll(new RegExp(r"^https?://"), ""); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Can you pull this out into a separate function and call it from here and line > 96? Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:124: return listDir(path.join(systemCacheRoot,url)).then((entries){ On 2013/03/12 22:12:03, Bob Nystrom wrote: > Space after "," and between ")" and "{". Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:131: return list; On 2013/03/12 22:12:03, Bob Nystrom wrote: > return entries.map((entry) { > var name = ... > return new Package.load... > }); Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:133: throw 'Could not list packages in pub cache ${ex}'; On 2013/03/12 22:12:03, Bob Nystrom wrote: > Don't bother catching this. Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/pub.dart File utils/pub/pub.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/pub.dart#newcode45 utils/pub/pub.dart:45: 'cache': new CacheCommand() On 2013/03/12 22:12:03, Bob Nystrom wrote: > In alphabetical order please. :) Done. https://codereview.chromium.org/12755024/diff/1/utils/pub/source.dart File utils/pub/source.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/pub/source.dart#newcode219 utils/pub/source.dart:219: /// Returns a list of [Package] that have been cached in in Pub's global cache On 2013/03/12 22:12:03, Bob Nystrom wrote: > Returns the [Package]s that have been installed in the system cache. Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:17: On 2013/03/12 22:12:03, Bob Nystrom wrote: > Remove this empty line. Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:18: servePackages([package("foo", "1.2.3")]); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Since you're actually creating the cache, you can simplify this a bunch. Remove > this line... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:18: servePackages([package("foo", "1.2.3")]); On 2013/03/12 22:12:03, Bob Nystrom wrote: > Since you're actually creating the cache, you can simplify this a bunch. Remove > this line... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:23: async(port.then((p) => dir('pub.dartlang.org', [ On 2013/03/12 22:12:03, Bob Nystrom wrote: > ...and async(port.then((p) => > > (since you don't use p) Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:27: ]) On 2013/03/12 22:12:03, Bob Nystrom wrote: > Let's add another package here too to make sure it lists them all. Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:33: appDir([dependency("foo", "1.2.3")]).scheduleCreate(); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ...and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:33: appDir([dependency("foo", "1.2.3")]).scheduleCreate(); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ...and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:36: output: new RegExp("Dependencies installed!\$")); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ..and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:38: cacheDir({"foo": "1.2.3"}).scheduleValidate(); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ..and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:38: cacheDir({"foo": "1.2.3"}).scheduleValidate(); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ..and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:38: cacheDir({"foo": "1.2.3"}).scheduleValidate(); On 2013/03/12 22:12:03, Bob Nystrom wrote: > ...and this... Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:41: new RegExp(r'{"pubcache":{"foo":{"version":"1.2.3","location":"[^"]+foo-1.2.3"}}}$')); On 2013/03/12 22:58:48, nweiz wrote: > On 2013/03/12 22:12:03, Bob Nystrom wrote: > > Instead of a RegExp, it's probably simpler here to just validate against a > > string literal: > > > > output: '{"pubcache...\n' > > I think a RegExp is right... the location can vary. The periods and curly braces > should be escaped, though. Done. https://codereview.chromium.org/12755024/diff/1/utils/tests/pub/pub_cache_tes... utils/tests/pub/pub_cache_test.dart:43: On 2013/03/12 22:12:03, Bob Nystrom wrote: > Add tests that: > > 1. It exits with the right error message if you run "pub cache" > 2. It exits with the right error message if you run "pub cache foo" > 3. It prints an empty map if there are no cached packages. Done.
Much better! https://codereview.chromium.org/12755024/diff/11001/utils/pub/command_cache.dart File utils/pub/command_cache.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/command_cache.d... utils/pub/command_cache.dart:44: // TODO(keertip): Add support for non-JSON format and check for --format flag Long line. https://codereview.chromium.org/12755024/diff/11001/utils/pub/git_source.dart File utils/pub/git_source.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/git_source.dart... utils/pub/git_source.dart:120: //TODO(keertip): to be impelemented. Remove the entire implementation (so that the throw from the base class will still happen) and just add a TODO in the class body: // TODO(keertip): Implement getCachedPackages(). https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:125: return new Package.load(null, entry, systemCache.sources); Only indent 2, or better, use => if it fits: return entries.map((entry) => new Package.load(null, entry, systemCache.sources)); https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:136: // }); Remove commented out code. :) https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:166: String _getSourceDirectory(String url){ Space after ) https://codereview.chromium.org/12755024/diff/11001/utils/tests/pub/pub_cache... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/tests/pub/pub_cache... utils/tests/pub/pub_cache_test.dart:41: dir('pub.dartlang.org', [ What happens if this directory doesn't exist?
https://codereview.chromium.org/12755024/diff/11001/utils/pub/command_cache.dart File utils/pub/command_cache.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/command_cache.d... utils/pub/command_cache.dart:44: // TODO(keertip): Add support for non-JSON format and check for --format flag On 2013/03/13 17:44:07, Bob Nystrom wrote: > Long line. Done. https://codereview.chromium.org/12755024/diff/11001/utils/pub/git_source.dart File utils/pub/git_source.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/git_source.dart... utils/pub/git_source.dart:120: //TODO(keertip): to be impelemented. On 2013/03/13 17:44:07, Bob Nystrom wrote: > Remove the entire implementation (so that the throw from the base class will > still happen) and just add a TODO in the class body: > > // TODO(keertip): Implement getCachedPackages(). Done. https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:125: return new Package.load(null, entry, systemCache.sources); On 2013/03/13 17:44:07, Bob Nystrom wrote: > Only indent 2, or better, use => if it fits: > > return entries.map((entry) => new Package.load(null, entry, > systemCache.sources)); Done. https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:136: // }); Oops! removed! On 2013/03/13 17:44:07, Bob Nystrom wrote: > Remove commented out code. :) https://codereview.chromium.org/12755024/diff/11001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:166: String _getSourceDirectory(String url){ On 2013/03/13 17:44:07, Bob Nystrom wrote: > Space after ) Done. https://codereview.chromium.org/12755024/diff/11001/utils/tests/pub/pub_cache... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/11001/utils/tests/pub/pub_cache... utils/tests/pub/pub_cache_test.dart:41: dir('pub.dartlang.org', [ Throws an exception. Added test for that. On 2013/03/13 17:44:07, Bob Nystrom wrote: > What happens if this directory doesn't exist?
https://codereview.chromium.org/12755024/diff/22001/utils/tests/pub/pub_cache... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/22001/utils/tests/pub/pub_cache... utils/tests/pub/pub_cache_test.dart:38: schedulePub(args: ['cache', 'list'], exitCode: 1); I think should actually pass. If you don't have a cache, that should be equivalent to an empty cache from the user's perspective. Pub should gracefully handle that case.
https://codereview.chromium.org/12755024/diff/22001/utils/tests/pub/pub_cache... File utils/tests/pub/pub_cache_test.dart (right): https://codereview.chromium.org/12755024/diff/22001/utils/tests/pub/pub_cache... utils/tests/pub/pub_cache_test.dart:38: schedulePub(args: ['cache', 'list'], exitCode: 1); On 2013/03/13 18:23:24, Bob Nystrom wrote: > I think should actually pass. If you don't have a cache, that should be > equivalent to an empty cache from the user's perspective. Pub should gracefully > handle that case. Done.
Almost there! https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:121: Future<List<Package>> getCachedPackages() { Since this method returns a future but internally does some synchronous IO, it needs to be wrapped in a defer() call. This ensures that an exception from dirExists() will propagate through the future. So do: getCachedPackages() { return defer(() { var url = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); if (!dirExists(url)) return []; ... }); } As a nice side-effect, you don't need the Future.immediate() anymore. You can just return [] and defer() will wrap it in a future for you. https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:122: var url = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); This isn't a url. How about "url" -> "cacheDir". https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:124: return new Future.immediate([]); Either put this on the same line as the if, or use a {} body.
https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:121: Future<List<Package>> getCachedPackages() { On 2013/03/13 21:10:43, Bob Nystrom wrote: > Since this method returns a future but internally does some synchronous IO, it > needs to be wrapped in a defer() call. This ensures that an exception from > dirExists() will propagate through the future. So do: > > getCachedPackages() { > return defer(() { > var url = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); > if (!dirExists(url)) return []; > ... > }); > } > > As a nice side-effect, you don't need the Future.immediate() anymore. You can > just return [] and defer() will wrap it in a future for you. Done. https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:122: var url = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); On 2013/03/13 21:10:43, Bob Nystrom wrote: > This isn't a url. How about "url" -> "cacheDir". Done. https://codereview.chromium.org/12755024/diff/28001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:124: return new Future.immediate([]); On 2013/03/13 21:10:43, Bob Nystrom wrote: > Either put this on the same line as the if, or use a {} body. Done.
One nit then LGTM! https://codereview.chromium.org/12755024/diff/36001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/36001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:123: var cacheDir = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); Long line.
https://codereview.chromium.org/12755024/diff/36001/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/12755024/diff/36001/utils/pub/hosted_source.d... utils/pub/hosted_source.dart:123: var cacheDir = path.join(systemCacheRoot, _getSourceDirectory(_defaultUrl)); fixed. On 2013/03/13 21:44:41, Bob Nystrom wrote: > Long line.
Message was sent while issue was closed.
Committed patchset #8 manually as r19977 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
