adds file path source to pub

adds file path source to pub


BUG=3732

[Bob Nystrom, 2013-01-17 19:15:10]

Despite the pile of review comments, this actually looks really nice. The main
missing piece is tests. I'd like to see a new "path" directory under
utils/tests/pub/install. In there, a couple of test files to test:

1. That installing an absolute path dependency works as expected. You can
probably use the SDK install tests as a reference here.
2. The same but using a relative path.
3. That installing a path dependency to a path that doesn't exist generates a
nice friendly error message.
4. That installing a path dependency where the path resolves to a file instead
of a directory generates a friendly error message.

There's also a corner case we need to worry about where the pubspec of the path
dependent package changes a constraint and pub may want to notice that and
handle it appropriately when you do a pub install. But I can file a bug for that
and handle it separately.

I can help with the tests if you like. I'd definitely like to get this in soon.
Thanks!

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

https://codereview.chromium.org/11931040/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:16: /// A package source that installs packages from
disk for development use
Add a "." to make this a full sentence. Also instead of "from disk", how about
"from a given file path".

https://codereview.chromium.org/11931040/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:19: final bool shouldCache = false;
Type annotations aren't needed for final fields, so just:

final name = 'path';
final shouldCache = false;

https://codereview.chromium.org/11931040/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:21: PathSource();
I think you can remove this if it doesn't do anything.

https://codereview.chromium.org/11931040/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:32: throw new FormatException("The description must
be a path string");
How about flipping this logic around and using if (description is! String)
throw...

Also, make the message a full sentence with a ".".

https://codereview.chromium.org/11931040/diff/1/utils/pub/validator/dependenc...
File utils/pub/validator/dependency.dart (right):

https://codereview.chromium.org/11931040/diff/1/utils/pub/validator/dependenc...
utils/pub/validator/dependency.dart:29: }
Let's reuse the _warnAboutSource method here since it generates a nice error
message. How about changing it to:

_reportInvalidSource(PackageRef ref, {bool error = false}) { ... }

Then in the body check "error" to see if it should be reported to errors or
warnings.

https://codereview.chromium.org/11931040/diff/1/utils/tests/pub/validator_tes...
File utils/tests/pub/validator_test.dart (right):

https://codereview.chromium.org/11931040/diff/1/utils/tests/pub/validator_tes...
utils/tests/pub/validator_test.dart:325: expectValidationError(dependency);
Great test.

[Sam E, 2013-01-20 04:16:57]

Bob,

I have added the tests as you recommended.

I did run into the following issues:
A. I could not figure out how to get the absolute path of the temp dir to
complete the absolute dir test.

B. The relative path test was failing. After digging around, I found that the
symlink that was getting created points to a relative path. This was causing the
failure since we were verifying from a different working directory. 
This was not an issue before since all cached paths are absolute.
I did fix the issue by enforcing the use of an absolute path in 'io.dart' 

If you think there is no side effects to my change in B. All what's left is to
figure out point A.

Thanks
Sam

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
File utils/pub/path_source.dart (right):

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
utils/pub/path_source.dart:16: /// A package source that installs packages from
disk for development use
On 2013/01/17 19:15:10, Bob Nystrom wrote:
> Add a "." to make this a full sentence. Also instead of "from disk", how about
> "from a given file path".

Done.

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
utils/pub/path_source.dart:16: /// A package source that installs packages from
disk for development use
On 2013/01/17 19:15:10, Bob Nystrom wrote:
> Add a "." to make this a full sentence. Also instead of "from disk", how about
> "from a given file path".

Done.

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
utils/pub/path_source.dart:19: final bool shouldCache = false;
On 2013/01/17 19:15:10, Bob Nystrom wrote:
> Type annotations aren't needed for final fields, so just:
> 
> final name = 'path';
> final shouldCache = false;

Done.

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
utils/pub/path_source.dart:21: PathSource();
On 2013/01/17 19:15:10, Bob Nystrom wrote:
> I think you can remove this if it doesn't do anything.

Done.

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/path_source....
utils/pub/path_source.dart:32: throw new FormatException("The description must
be a path string");
On 2013/01/17 19:15:10, Bob Nystrom wrote:
> How about flipping this logic around and using if (description is! String)
> throw...
> 
> Also, make the message a full sentence with a ".".

Done.

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/validator/de...
File utils/pub/validator/dependency.dart (right):

https://chromiumcodereview.appspot.com/11931040/diff/1/utils/pub/validator/de...
utils/pub/validator/dependency.dart:29: }
> Let's reuse the _warnAboutSource method here since it generates a nice error
> message. How about changing it to:

I don't think this is going to work. The logic is completely different when
using a path source.

I'll leave it as is for now

[Bob Nystrom, 2013-01-23 00:49:19]

This looks great! There's a few loose ends left, but I think I can take it from
here. Before I do, though, there's one thing: it looks like the branch you
uploaded from is out of sync with master. Can you rebase against master and then
upload the patch again?

Your patch has changes in the diff here that aren't changes you made, like the
"integration()" stuff.

Thanks!

[Sam E, 2013-01-23 12:02:01]

I did another git cl upload. Same thing.
I see where the confusion for the "integration()" stuff is coming from.

For some reason git cl thinks I moved files from another directory, here is the
output from git cl upload:

 utils/pub/io.dart                                                              
                                |  4 +++-
 utils/pub/path_source.dart                                                     
                                | 33 +++++++++++++++++++++++++++++++++
 utils/pub/system_cache.dart                                                    
                                |  4 +++-
 utils/pub/validator/dependency.dart                                            
                                |  8 +++++++-
 utils/tests/pub/install/{git/different_repo_name_test.dart =>
path/absolute_path_test.dart}                     | 24 ++++++++++++------------
 utils/tests/pub/install/{hosted/fail_gracefully_on_url_resolve_test.dart =>
path/path_does_not_exist_test.dart} | 26 ++++++++++++++------------
 utils/tests/pub/install/{hosted/fail_gracefully_on_url_resolve_test.dart =>
path/path_is_file_test.dart}        | 26 ++++++++++++++------------
 utils/tests/pub/install/{git/different_repo_name_test.dart =>
path/relative_path_test.dart}                     | 24 ++++++++++++------------
 utils/tests/pub/test_pub.dart                                                  
                                |  6 +++++-
 utils/tests/pub/validator_test.dart                                            
                                | 11 ++++++++++-
 10 files changed, 113 insertions(+), 53 deletions(-)

any reason why this is happening?
Thanks

[Bob Nystrom, 2013-01-24 18:16:35]

On 2013/01/23 12:02:01, Sam E wrote:
> I did another git cl upload. Same thing.
> I see where the confusion for the "integration()" stuff is coming from.
> 
> For some reason git cl thinks I moved files from another directory, here is
the
> output from git cl upload:
> 
>  utils/pub/io.dart                                                            
 
>                                 |  4 +++-
>  utils/pub/path_source.dart                                                   
 
>                                 | 33 +++++++++++++++++++++++++++++++++
>  utils/pub/system_cache.dart                                                  
 
>                                 |  4 +++-
>  utils/pub/validator/dependency.dart                                          
 
>                                 |  8 +++++++-
>  utils/tests/pub/install/{git/different_repo_name_test.dart =>
> path/absolute_path_test.dart}                     | 24
++++++++++++------------
>  utils/tests/pub/install/{hosted/fail_gracefully_on_url_resolve_test.dart =>
> path/path_does_not_exist_test.dart} | 26 ++++++++++++++------------
>  utils/tests/pub/install/{hosted/fail_gracefully_on_url_resolve_test.dart =>
> path/path_is_file_test.dart}        | 26 ++++++++++++++------------
>  utils/tests/pub/install/{git/different_repo_name_test.dart =>
> path/relative_path_test.dart}                     | 24
++++++++++++------------
>  utils/tests/pub/test_pub.dart                                                
 
>                                 |  6 +++++-
>  utils/tests/pub/validator_test.dart                                          
 
>                                 | 11 ++++++++++-
>  10 files changed, 113 insertions(+), 53 deletions(-)
> 
> any reason why this is happening?
> Thanks

I think Reitveld (the code review site) tries to detect renames and show them
differently. The new test files are similar to some existing tests, so it's
getting confused. I can probably sort the patch out on my end.

I'm mired in some other stuff right now, but I'll try to take a look at this
soon. Cheers!