Make pub publish more user friendly:

Make pub publish more user friendly:

- Show the contents of their package.
- Let the confirm the upload even if there are no warnings.
- Make the error/warning text less scary.
- Validate that the pubspec has a version client-side.

Addresses #7175 and #7219.

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

[Bob Nystrom, 2012-12-12 07:39:33]

This is a bigger and scarier patch than I'd like right before the release. I
definitely want to dogfood this a bit and make sure it's working OK. My changes
to the tests are a bit hairy too. Tests that involve user input in pub are still
a little rough.

But, I think the end user experience is much nicer. You'll get something like:

Publishing "path" 0.0.0:
|-- .gitignore
|-- COPYING
|-- README.md
|-- example
|   |-- console_example.dart
|   |-- main.dart
|   '-- web copy
|       '-- web_example.dart
|-- lib
|   |-- main.dart
|   '-- path.dart
|-- pubspec.yaml
'-- test
    |-- absolute_test.dart
    |-- all_test.dart
    |-- basename_test.dart
    |   (7 more...)
    |-- path_windows_test.dart
    |-- relative_test.dart
    '-- split_test.dart

Missing requirements:
* Your pubspec.yaml is missing a "version" field.

Suggestions:
* Author "bob" in pubspec.yaml should have an email address
  (e.g. "name <email>").

Sorry, your package is missing a requirement and can't be published yet.
For more information, see: http://pub.dartlang.org/doc/pub-lish.html.

[nweiz, 2012-12-12 21:21:21]

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

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:42: return
client.get(server.resolve("/packages/versions/new.json")).chain((response) {
Long line

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:113:
}).chain(consumeInputStream).chain((packageBytes) {
Why are we blocking on creating the tarball? Can't we do that in the background
while confirmation and validation are taking place?

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:126: final _BLACKLISTED_FILES = const ['.DS_Store',
'pubspec.lock'];
Adding .DS_Store here is unnecessary; hidden files are blacklisted by default
outside of Git, and on Git this is usually going to be gitignored.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:154: return relativeTo(entry, rootDir);
I'm worried that this will cause tricky bugs in the future. Right now the CWD is
always the package root, but if that ever changes this will stop working with
createTarGz. I think we should just call relativeTo in places where we need the
relative paths, even if that means we call it multiple times.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.dart
File utils/pub/directory_tree.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:56: ///         '-- split_test.dart
This is really nice-looking.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:58: String generateTree(List<String> files) {
I feel like this should have the same signature as createTarGz (that is, taking
arbitrary paths and relativizing them within the function).

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:79: if (isLastChild == true) {
if (isLastChild)

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:81: } else if (isLastChild == false) {
if (!isLastChild)

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:90: String _getPrefix(bool isLast) {
Tri-state booleans are awful. Just pass in two separate parameters.

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

https://codereview.chromium.org/11557008/diff/3001/utils/pub/io.dart#newcode474
utils/pub/io.dart:474: /// do not.
It would be nice to document that the message should be in the form of a
question, and that it shouldn't have any trailing punctuation.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/path.dart
File utils/pub/path.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/path.dart#newcod...
utils/pub/path.dart:124: List<String> split(String path) =>
_builder.split(path);
I assume this'll be replaced by my implementation.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/lib.dart
File utils/pub/validator/lib.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/lib.da...
utils/pub/validator/lib.dart:39: "Otherwise, users cannot import any code from
your package.");
Nit: why "Otherwise" here and "Without that" in the other messages?

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/licens...
File utils/pub/validator/license.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/licens...
utils/pub/validator/license.dart:24: "An open-source license helps ensure people
can legally use your code.");
Long lines.

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

https://codereview.chromium.org/11557008/diff/3001/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:635: void confirmPublish(ScheduledProcess pub) {
I don't like that this only works with a specific package structure. Maybe just
validate that the first line matches 'Publishing ".*" [0-9]\.[0-9]\.[0-9]:",
then add some sort of ScheduledProcess.ignoreStdoutUntil function and use that?

You could also just add a TODO.

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

https://codereview.chromium.org/11557008/diff/5001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:219: message = "Package has ${warnings.length}
warning$s. Upload anyway";
It's potentially confusing that warnings are referred to as "warnings" here but
are displayed as "suggestions".

[Bob Nystrom, 2012-12-12 21:45:36]

Thanks!

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

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:42: return
client.get(server.resolve("/packages/versions/new.json")).chain((response) {
On 2012/12/12 21:21:21, nweiz wrote:
> Long line

Done.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:113:
}).chain(consumeInputStream).chain((packageBytes) {
On 2012/12/12 21:21:21, nweiz wrote:
> Why are we blocking on creating the tarball? Can't we do that in the
background
> while confirmation and validation are taking place?

Yesterday, you mentioned wanting to eventually validate the size of the archive,
so I went ahead and moved it first in the process. We could do it in parallel
too, but I was trying to keep this patch as easy (for me at least) to understand
as I could.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:126: final _BLACKLISTED_FILES = const ['.DS_Store',
'pubspec.lock'];
On 2012/12/12 21:21:21, nweiz wrote:
> Adding .DS_Store here is unnecessary; hidden files are blacklisted by default
> outside of Git, and on Git this is usually going to be gitignored.

Done.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:154: return relativeTo(entry, rootDir);
On 2012/12/12 21:21:21, nweiz wrote:
> I'm worried that this will cause tricky bugs in the future. Right now the CWD
is
> always the package root, but if that ever changes this will stop working with
> createTarGz. I think we should just call relativeTo in places where we need
the
> relative paths, even if that means we call it multiple times.

Good point. It does work now and I'd like to get this patch in as quickly as
possible, so I added a TODO.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.dart
File utils/pub/directory_tree.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:56: ///         '-- split_test.dart
On 2012/12/12 21:21:21, nweiz wrote:
> This is really nice-looking.

Thanks!

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:58: String generateTree(List<String> files) {
On 2012/12/12 21:21:21, nweiz wrote:
> I feel like this should have the same signature as createTarGz (that is,
taking
> arbitrary paths and relativizing them within the function).

So passing in the root directory? Otherwise, how does it know what to relativize
them against?

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:79: if (isLastChild == true) {
On 2012/12/12 21:21:21, nweiz wrote:
> if (isLastChild)

It's nullable. But fixed.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:81: } else if (isLastChild == false) {
On 2012/12/12 21:21:21, nweiz wrote:
> if (!isLastChild)

Done.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:90: String _getPrefix(bool isLast) {
On 2012/12/12 21:21:21, nweiz wrote:
> Tri-state booleans are awful. Just pass in two separate parameters.

Done.

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

https://codereview.chromium.org/11557008/diff/3001/utils/pub/io.dart#newcode474
utils/pub/io.dart:474: /// do not.
On 2012/12/12 21:21:21, nweiz wrote:
> It would be nice to document that the message should be in the form of a
> question, and that it shouldn't have any trailing punctuation.

Done.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/path.dart
File utils/pub/path.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/path.dart#newcod...
utils/pub/path.dart:124: List<String> split(String path) =>
_builder.split(path);
On 2012/12/12 21:21:21, nweiz wrote:
> I assume this'll be replaced by my implementation.

Yup.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/lib.dart
File utils/pub/validator/lib.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/lib.da...
utils/pub/validator/lib.dart:39: "Otherwise, users cannot import any code from
your package.");
On 2012/12/12 21:21:21, nweiz wrote:
> Nit: why "Otherwise" here and "Without that" in the other messages?

It felt weird to me to have "without that" referring to "something other than".
"That" seems to refer to something specific, and "something other than" isn't
very.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/licens...
File utils/pub/validator/license.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/validator/licens...
utils/pub/validator/license.dart:24: "An open-source license helps ensure people
can legally use your code.");
On 2012/12/12 21:21:21, nweiz wrote:
> Long lines.

Done.

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

https://codereview.chromium.org/11557008/diff/3001/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:635: void confirmPublish(ScheduledProcess pub) {
On 2012/12/12 21:21:21, nweiz wrote:
> I don't like that this only works with a specific package structure. Maybe
just
> validate that the first line matches 'Publishing ".*" [0-9]\.[0-9]\.[0-9]:",
> then add some sort of ScheduledProcess.ignoreStdoutUntil function and use
that?
> 
> You could also just add a TODO.

Agreed completely. Added a TODO.

[nweiz, 2012-12-12 21:59:51]

lgtm

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

https://codereview.chromium.org/11557008/diff/3001/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:113:
}).chain(consumeInputStream).chain((packageBytes) {
On 2012/12/12 21:45:36, Bob Nystrom wrote:
> On 2012/12/12 21:21:21, nweiz wrote:
> > Why are we blocking on creating the tarball? Can't we do that in the
> background
> > while confirmation and validation are taking place?
> 
> Yesterday, you mentioned wanting to eventually validate the size of the
archive,
> so I went ahead and moved it first in the process. We could do it in parallel
> too, but I was trying to keep this patch as easy (for me at least) to
understand
> as I could.

I was thinking we'd just pass the archive future into the validator somehow.
I'll handle that in the patch that adds the validator, though.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.dart
File utils/pub/directory_tree.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:58: String generateTree(List<String> files) {
On 2012/12/12 21:45:36, Bob Nystrom wrote:
> On 2012/12/12 21:21:21, nweiz wrote:
> > I feel like this should have the same signature as createTarGz (that is,
> taking
> > arbitrary paths and relativizing them within the function).
> 
> So passing in the root directory? Otherwise, how does it know what to
relativize
> them against?

Yes. createTarGz takes a list of files and a base path.

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:79: if (isLastChild == true) {
On 2012/12/12 21:45:36, Bob Nystrom wrote:
> On 2012/12/12 21:21:21, nweiz wrote:
> > if (isLastChild)
> 
> It's nullable. But fixed.

In practice, I don't think null is ever passed in here -- the "name != null"
check in _draw protects against that. Also, I don't think it should be nullable.

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

https://codereview.chromium.org/11557008/diff/6017/utils/pub/command_lish.dar...
utils/pub/command_lish.dart:163: // creation if the cwd is never *not* the
package root directory.
Nit: "never" -> "ever"

[Bob Nystrom, 2012-12-12 22:14:32]

Thanks!

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.dart
File utils/pub/directory_tree.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:79: if (isLastChild == true) {
On 2012/12/12 21:59:51, nweiz wrote:
> On 2012/12/12 21:45:36, Bob Nystrom wrote:
> > On 2012/12/12 21:21:21, nweiz wrote:
> > > if (isLastChild)
> > 
> > It's nullable. But fixed.
> 
> In practice, I don't think null is ever passed in here -- the "name != null"
> check in _draw protects against that. Also, I don't think it should be
nullable.

The name != null check is new. That's how I avoid making it nullable now. :)

[nweiz, 2012-12-12 22:19:33]

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.dart
File utils/pub/directory_tree.dart (right):

https://codereview.chromium.org/11557008/diff/3001/utils/pub/directory_tree.d...
utils/pub/directory_tree.dart:79: if (isLastChild == true) {
On 2012/12/12 22:14:32, Bob Nystrom wrote:
> On 2012/12/12 21:59:51, nweiz wrote:
> > On 2012/12/12 21:45:36, Bob Nystrom wrote:
> > > On 2012/12/12 21:21:21, nweiz wrote:
> > > > if (isLastChild)
> > > 
> > > It's nullable. But fixed.
> > 
> > In practice, I don't think null is ever passed in here -- the "name != null"
> > check in _draw protects against that. Also, I don't think it should be
> nullable.
> 
> The name != null check is new. That's how I avoid making it nullable now. :)

I meant the one in _draw. _drawLine is never called with a null name.