Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12226077:
add --preview flag to publish command (Closed)
|
Created:
7 years, 10 months ago by keertip Modified:
7 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionadd --preview flag to publish command
Committed: https://code.google.com/p/dart/source/detail?r=18397
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:36: help: 'Preview publishing the package'); Weren't we going to call this --dry-run? https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:171: log.warning("Package has ${warnings.length} warnings."); Nit: it would be nice to do the pluralization trick we do above so it doesn't say "1 warnings". https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:173: } Style nit: I'd move this block above the message assignment. https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... utils/tests/pub/pub_lish_test.dart:97: integration('preview package validation has a warning', () { It would be good to test the no-warnings case as well. https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... utils/tests/pub/pub_lish_test.dart:109: contains('Suggestions:\n* Author "Nathan Weizenbaum" in pubspec.yaml should have an email address\n' Line length
https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:36: help: 'Preview publishing the package'); Not sure what we settled on in the end, but am OK with what you and Bob decide. On 2013/02/09 00:20:50, nweiz wrote: > Weren't we going to call this --dry-run? https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:171: log.warning("Package has ${warnings.length} warnings."); On 2013/02/09 00:20:50, nweiz wrote: > Nit: it would be nice to do the pluralization trick we do above so it doesn't > say "1 warnings". Done. https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:173: } On 2013/02/09 00:20:50, nweiz wrote: > Style nit: I'd move this block above the message assignment. Done. https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... utils/tests/pub/pub_lish_test.dart:97: integration('preview package validation has a warning', () { On 2013/02/09 00:20:50, nweiz wrote: > It would be good to test the no-warnings case as well. Done. https://codereview.chromium.org/12226077/diff/1/utils/tests/pub/pub_lish_test... utils/tests/pub/pub_lish_test.dart:109: contains('Suggestions:\n* Author "Nathan Weizenbaum" in pubspec.yaml should have an email address\n' On 2013/02/09 00:20:50, nweiz wrote: > Line length Done.
LGTM mod a few remaining comments. https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:36: help: 'Preview publishing the package'); On 2013/02/09 00:53:40, keertip wrote: > Not sure what we settled on in the end, but am OK with what you and Bob decide. > > On 2013/02/09 00:20:50, nweiz wrote: > > Weren't we going to call this --dry-run? > I'd prefer --dry-run, since that's consistent with Git and other command-line tools. Bob isn't here to give his opinion, but I think he agreed last time we talked about this. https://codereview.chromium.org/12226077/diff/7001/utils/tests/pub/pub_lish_t... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/7001/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:126: contains('')); Shouldn't this check for 'Package has 0 warnings.'?
https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/1/utils/pub/command_lish.dart#n... utils/pub/command_lish.dart:36: help: 'Preview publishing the package'); On 2013/02/09 00:59:02, nweiz wrote: > On 2013/02/09 00:53:40, keertip wrote: > > Not sure what we settled on in the end, but am OK with what you and Bob > decide. > > > > On 2013/02/09 00:20:50, nweiz wrote: > > > Weren't we going to call this --dry-run? > > > > I'd prefer --dry-run, since that's consistent with Git and other command-line > tools. Bob isn't here to give his opinion, but I think he agreed last time we > talked about this. Done.
https://codereview.chromium.org/12226077/diff/7001/utils/tests/pub/pub_lish_t... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/7001/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:126: contains('')); Yes, missed that - fixed. On 2013/02/09 00:59:02, nweiz wrote: > Shouldn't this check for 'Package has 0 warnings.'?
https://codereview.chromium.org/12226077/diff/2002/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/pub/command_lish.dar... utils/pub/command_lish.dart:166: exit(0); We're trying to move away from explicitly calling exit() since that will do bad things at some point in the future when dart:io makes stdin/stdout async. Instead, can you make _validate return a Future<bool> and complete to true if the publish should proceed and false if it shouldn't? Then onRun can just check that and not call _publish if it's false. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:104: lishArgs.add("--dry-run"); Eek, Java! :) Just do: startPubLish(server, args: ['--dry-run']); https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:105: var pub = startPubLish(server,args: lishArgs); Space after ",". https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:107: pub.shouldExit(0); Do we want a zero exit code for this? It might be helpful if it returned non-zero on validation failure. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:110: 'should have an email address\n (e.g. "name <email>").\n\n' Extra leading space here. Also, I'd split it at the "\n" just so that the code matches the output a bit more: 'should have an email address\n' ' (e.g. "name <email>").\n\n' https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:122: var pub = startPubLish(server,args: lishArgs); lishArgs -> ['--dry-run'] https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_test.dart File utils/tests/pub/pub_test.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_test.d... utils/tests/pub/pub_test.dart:108: -n, --dry-run Preview publishing the package "Validate but do not publish the package"
https://codereview.chromium.org/12226077/diff/2002/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/pub/command_lish.dar... utils/pub/command_lish.dart:166: exit(0); On 2013/02/11 18:44:37, Bob Nystrom wrote: > We're trying to move away from explicitly calling exit() since that will do bad > things at some point in the future when dart:io makes stdin/stdout async. > > Instead, can you make _validate return a Future<bool> and complete to true if > the publish should proceed and false if it shouldn't? > > Then onRun can just check that and not call _publish if it's false. Done. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:104: lishArgs.add("--dry-run"); On 2013/02/11 18:44:37, Bob Nystrom wrote: > Eek, Java! :) > > Just do: > > startPubLish(server, args: ['--dry-run']); Done. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:105: var pub = startPubLish(server,args: lishArgs); On 2013/02/11 18:44:37, Bob Nystrom wrote: > Space after ",". Done. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_lish_t... utils/tests/pub/pub_lish_test.dart:122: var pub = startPubLish(server,args: lishArgs); On 2013/02/11 18:44:37, Bob Nystrom wrote: > lishArgs -> ['--dry-run'] Done. https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_test.dart File utils/tests/pub/pub_test.dart (right): https://codereview.chromium.org/12226077/diff/2002/utils/tests/pub/pub_test.d... utils/tests/pub/pub_test.dart:108: -n, --dry-run Preview publishing the package On 2013/02/11 18:44:37, Bob Nystrom wrote: > "Validate but do not publish the package" Done.
https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:37: "Validate but do not publish the package"); You can remove the "Preview..." line. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:106: .then((validate) { "validate" -> "isValid" https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:108: }); The styling of this is a bit strange. How about: return _validate(... .then((isValid) { if (isValid) return package... }); https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:163: "http://pub.dartlang.org/doc/pub-lish.html.\n"; Instead of throwing, just use log.error() to print the message then return false. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:169: return new Future.immediate(false); You don't need Future here, just "return false;" https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:180: if (!confirmed) throw "Package upload canceled."; Instead of throwing here, do log.message() then return false. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:181: return new Future.immediate(true); Just "return true". https://codereview.chromium.org/12226077/diff/10002/utils/tests/pub/pub_lish_... File utils/tests/pub/pub_lish_test.dart (right): https://codereview.chromium.org/12226077/diff/10002/utils/tests/pub/pub_lish_... utils/tests/pub/pub_lish_test.dart:123: contains('Package has 0 warnings.')); Will this fit on one line?
https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:37: "Validate but do not publish the package"); On 2013/02/12 01:00:41, Bob Nystrom wrote: > You can remove the "Preview..." line. Done. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:106: .then((validate) { On 2013/02/12 01:00:41, Bob Nystrom wrote: > "validate" -> "isValid" Done. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:108: }); On 2013/02/12 01:00:41, Bob Nystrom wrote: > The styling of this is a bit strange. How about: > > return _validate(... > .then((isValid) { > if (isValid) return package... > }); Done. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:163: "http://pub.dartlang.org/doc/pub-lish.html.\n"; With this change, publish will return with exitcode 0 even when there are erros in the package. Would this be ok? Do we want to have a different exitcode to indicate errors? On 2013/02/12 01:00:41, Bob Nystrom wrote: > Instead of throwing, just use log.error() to print the message then return > false. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:169: return new Future.immediate(false); On 2013/02/12 01:00:41, Bob Nystrom wrote: > You don't need Future here, just "return false;" Done. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:180: if (!confirmed) throw "Package upload canceled."; On 2013/02/12 01:00:41, Bob Nystrom wrote: > Instead of throwing here, do log.message() then return false. Done. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:181: return new Future.immediate(true); On 2013/02/12 01:00:41, Bob Nystrom wrote: > Just "return true". Done.
Have you uploaded changes? I see comment replies but no code difference. https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.dart File utils/pub/command_lish.dart (right): https://codereview.chromium.org/12226077/diff/10002/utils/pub/command_lish.da... utils/pub/command_lish.dart:163: "http://pub.dartlang.org/doc/pub-lish.html.\n"; On 2013/02/12 05:20:20, keertip wrote: > With this change, publish will return with exitcode 0 even when there are erros > in the package. Would this be ok? Do we want to have a different exitcode to > indicate errors? Hmm. Good point. I think I'm OK with that for now at least.
PTAL - sorry about that, sources have been uploaded.
LGTM! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
