Add an initial "pub lish" command.

Add an initial "pub lish" command.

BUG=6177
Committed: https://code.google.com/p/dart/source/detail?r=15422

[Bob Nystrom, 2012-11-26 23:39:52]

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:30: /// The URL of the server to which to upload the
package.
Don't fear the dangling preposition. "The URL of the server to upload the
package to." is fine. :)

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:56: if (url is! String)
_invalidServerResponse(response);
How about moving the condition into the function, so you can do:

_expectResponse(url is String, response);

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:94: "be automatically refreshed.");
Can we tell the user what they need to do to solve this problem?

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:100: /// If this is a Git repository, this will
respect .gitignore; otherwise, it
Brilliant.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:110: // to Git.
Should we warn if they are about to publish files that aren't also checked into
the repo?

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:121: return files.map((file) {
What happens on Windows here?

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:135: } catch (e) {
Bare catches make me break out in hives. This is because of JSON being dumb,
right? If so, add a TODO to fix this when JSON is fixed.

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode373
utils/pub/io.dart:373: StringInputStream _stringStdin = new
StringInputStream(stdin);
"StringInputStream" -> "final".

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode776
utils/pub/io.dart:776: args.addAll(contents.map(_getPath));
Do we need to worry about blowing the arg buffer here if the package is large?

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode784
utils/pub/io.dart:784: withTempDir((tempDir) {
Yay Windows support!

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode10
utils/pub/oauth2.dart:10: import '../../pkg/oauth2/lib/oauth2.dart';
Add a TODO here to do... something... better than this path at some point. Not
sure what.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode19
utils/pub/oauth2.dart:19: final String _identifier =
'818368855108-8grd2eg9tj9f38os6f1urbcvsq399u8n.apps.'
You don't need to type annotate final top-level variables, since it's always
obvious from the initializer.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode131
utils/pub/oauth2.dart:131: "can't be automatically refreshed.");
We should tell the user what they need to do here.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode140
utils/pub/oauth2.dart:140: 'Obtaining new credentials...');
I don't understand this bit. Where is it obtaining the new credentials?

Also, we should really guard this by checking that "e" is some expected type.

[nweiz, 2012-11-27 20:15:54]

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:30: /// The URL of the server to which to upload the
package.
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Don't fear the dangling preposition. "The URL of the server to upload the
> package to." is fine. :)

Never. :D

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:56: if (url is! String)
_invalidServerResponse(response);
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> How about moving the condition into the function, so you can do:
> 
> _expectResponse(url is String, response);

It doesn't really seem worthwhile to move a one-line bit of logic into a
function.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:94: "be automatically refreshed.");
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Can we tell the user what they need to do to solve this problem?

We restart the command immediately afterwards, which will prompt the user to
re-authorize.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:110: // to Git.
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Should we warn if they are about to publish files that aren't also checked
into
> the repo?

I don't think so. It seems like that would get really annoying if there were
consistently something you wanted to publish that wasn't checked in. Also, I
think the mental model of "check in everything but these files" makes sense.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:121: return files.map((file) {
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> What happens on Windows here?

As far as I can tell it should just work.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:135: } catch (e) {
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Bare catches make me break out in hives. This is because of JSON being dumb,
> right? If so, add a TODO to fix this when JSON is fixed.

Done.

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode373
utils/pub/io.dart:373: StringInputStream _stringStdin = new
StringInputStream(stdin);
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> "StringInputStream" -> "final".

Done.

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode776
utils/pub/io.dart:776: args.addAll(contents.map(_getPath));
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Do we need to worry about blowing the arg buffer here if the package is large?

I think the buffer is really big on modern systems, but I'll add a TODO for it.

https://codereview.chromium.org/11308212/diff/1/utils/pub/io.dart#newcode784
utils/pub/io.dart:784: withTempDir((tempDir) {
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Yay Windows support!

I just more or less copied your code from test_pub. Which reminds me, I should
make that code use this function.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode10
utils/pub/oauth2.dart:10: import '../../pkg/oauth2/lib/oauth2.dart';
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> Add a TODO here to do... something... better than this path at some point. Not
> sure what.

Done.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode19
utils/pub/oauth2.dart:19: final String _identifier =
'818368855108-8grd2eg9tj9f38os6f1urbcvsq399u8n.apps.'
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> You don't need to type annotate final top-level variables, since it's always
> obvious from the initializer.

Done.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode131
utils/pub/oauth2.dart:131: "can't be automatically refreshed.");
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> We should tell the user what they need to do here.

Returning null here will trigger the re-authorization process.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode140
utils/pub/oauth2.dart:140: 'Obtaining new credentials...');
On 2012/11/26 23:39:52, Bob Nystrom wrote:
> I don't understand this bit. Where is it obtaining the new credentials?

The "null" return value is a signal that the credentials should be reloaded.

> Also, we should really guard this by checking that "e" is some expected type.

This is one case where I sort of feel like it makes sense to have a broad catch.
There are a wide variety of exceptions that could be thrown in this bit of code,
up to and including crazy things like dart:io bugs, and we want to recover from
almost all of them. Rather than try to enumerate everything that could possibly
go wrong in the previous block of code and risk missing a case and breaking
someone's ability to upload, it seems safer to catch everything and risk trying
to recover an unrecoverable condition.

[Bob Nystrom, 2012-11-27 21:00:53]

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:30: /// The URL of the server to which to upload the
package.
On 2012/11/27 20:15:54, nweiz wrote:
> On 2012/11/26 23:39:52, Bob Nystrom wrote:
> > Don't fear the dangling preposition. "The URL of the server to upload the
> > package to." is fine. :)
> 
> Never. :D

Dangling prepositions are a grammatical offense up with which you will not put!

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:56: if (url is! String)
_invalidServerResponse(response);
On 2012/11/27 20:15:54, nweiz wrote:
> On 2012/11/26 23:39:52, Bob Nystrom wrote:
> > How about moving the condition into the function, so you can do:
> > 
> > _expectResponse(url is String, response);
> 
> It doesn't really seem worthwhile to move a one-line bit of logic into a
> function.

Fair enough. Part of my motivation here is that I'd prefer a more verby method
name. But suit yourself.

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:125: });
Oh, I forgot to mention this in the original review. It would probably be good
to explicitly filter out "packages" directories here. We want to make sure we
don't upload them even if the user doesn't happen to have a .gitignore that
strips them out.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode140
utils/pub/oauth2.dart:140: 'Obtaining new credentials...');
On 2012/11/27 20:15:54, nweiz wrote:
> On 2012/11/26 23:39:52, Bob Nystrom wrote:
> > I don't understand this bit. Where is it obtaining the new credentials?
> 
> The "null" return value is a signal that the credentials should be reloaded.

Can you add a comment with that?

> 
> > Also, we should really guard this by checking that "e" is some expected
type.
> 
> This is one case where I sort of feel like it makes sense to have a broad
catch.
> There are a wide variety of exceptions that could be thrown in this bit of
code,
> up to and including crazy things like dart:io bugs, and we want to recover
from
> almost all of them. Rather than try to enumerate everything that could
possibly
> go wrong in the previous block of code and risk missing a case and breaking
> someone's ability to upload, it seems safer to catch everything and risk
trying
> to recover an unrecoverable condition.

I'm fine with broadly catching runtime exceptions. What I'm not crazy about is
that this will also catch system and programmatic errors. This will swallow
NoSuchMethodError, ArgumentError, StackOverflowError, etc. which seems pretty
wrong to me.

It would be nice if our exception hierarchy had a single type (Exception) that
was the supertype of all runtime errors but not programmatic ones, but we don't
currently.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.dart
File utils/tests/pub/oauth2_test.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:28: server.handle('GET',
'/packages/versions/new.json', (request, response) {
These callbacks need to be wrapped in expectAsync or guardAsync, right?
Otherwise it will do nasty things if the test fails.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:160: void
handleAccessTokenRequest(ScheduledServer server, String accessToken) {
What's the difference between this and the code at #69?

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/pub_test.dart
File utils/tests/pub/pub_test.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/pub_test.dart...
utils/tests/pub/pub_test.dart:25: lish      publish the current package to
pub.dartlang.org
As awesome as "lish" is, we should make "publish" the default command name.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1313: // Sleep for half a second in case
_endExpected is set in the next
Why 500ms?

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1393: if (expectedExitCode != null)
expect(exitCode, equals(expectedExitCode));
Long line.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1451: _schedule((_) {
Could do:

_schedule((_) => timeout(...));

And get rid of a level of indentation.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1511: _schedule((parentDir) {
"parentDir" -> "value".

[nweiz, 2012-11-27 22:07:34]

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:125: });
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> Oh, I forgot to mention this in the original review. It would probably be good
> to explicitly filter out "packages" directories here. We want to make sure we
> don't upload them even if the user doesn't happen to have a .gitignore that
> strips them out.

Done.

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/pub/oauth2.dart#newcode140
utils/pub/oauth2.dart:140: 'Obtaining new credentials...');
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> Can you add a comment with that?

Done.

> I'm fine with broadly catching runtime exceptions. What I'm not crazy about is
> that this will also catch system and programmatic errors. This will swallow
> NoSuchMethodError, ArgumentError, StackOverflowError, etc. which seems pretty
> wrong to me.
> 
> It would be nice if our exception hierarchy had a single type (Exception) that
> was the supertype of all runtime errors but not programmatic ones, but we
don't
> currently.

It's not like those errors will be swallowed silently; they'll still print the
warning message. This will just allow the user to continue if such an error
occurs in this non-essential bit of code.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.dart
File utils/tests/pub/oauth2_test.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:28: server.handle('GET',
'/packages/versions/new.json', (request, response) {
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> These callbacks need to be wrapped in expectAsync or guardAsync, right?
> Otherwise it will do nasty things if the test fails.

It's actually important that they're *not*. They're already set up as part of
the schedule, so errors are piped through there to allow its cleanup code to
run. If the plain unittest code caught the error, the cleanup code wouldn't run
at all.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:160: void
handleAccessTokenRequest(ScheduledServer server, String accessToken) {
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> What's the difference between this and the code at #69?

That code handles a refresh request, while this code handles an initial request.
The only code difference is what body is expected.

If we had more tests that requested a refresh it might be worth unifying them,
but as is I think it makes sense for the one-off behavior change to have a
little duplication.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/pub_test.dart
File utils/tests/pub/pub_test.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/pub_test.dart...
utils/tests/pub/pub_test.dart:25: lish      publish the current package to
pub.dartlang.org
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> As awesome as "lish" is, we should make "publish" the default command name.

That's part of issue 6949.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1313: // Sleep for half a second in case
_endExpected is set in the next
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> Why 500ms?

It seemed like a time increment that would be large enough to allow the
previously scheduled event to finish up (assuming it's done with most of its IO)
while being short enough to avoid overlapping too much with future callbacks and
having a false positive due to _endExpected being set later.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1393: if (expectedExitCode != null)
expect(exitCode, equals(expectedExitCode));
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1451: _schedule((_) {
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> Could do:
> 
> _schedule((_) => timeout(...));
> 
> And get rid of a level of indentation.

That's allowed? :o

https://codereview.chromium.org/11308212/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:1511: _schedule((parentDir) {
On 2012/11/27 21:00:53, Bob Nystrom wrote:
> "parentDir" -> "value".

This isn't the value that's being returned. It actually is the parent directory
of the test.

[Bob Nystrom, 2012-11-27 22:42:02]

LGTM!