Add Resource package.

Add Resource package.

R=floitsch@google.com

Committed: 527b11d472abbdc4ee3d08749f5685e2265f3465

[Lasse Reichstein Nielsen, 2015-10-06 12:45:23]

lrn@google.com changed reviewers:
+ floitsch@google.com

[Lasse Reichstein Nielsen, 2015-10-06 12:45:23]

I still need to find a good way to test dart:io specific functionality.

[Lasse Reichstein Nielsen, 2015-10-08 08:51:17]

lrn@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2015-10-08 16:48:34]

What do you think of changing the package name to "resources"?

One mistake I've made in the past is giving a package a name that collides with
a likely variable name that someone using the package would declare. That's why
most people import the path package as a "p" prefix. :( I wish we'd named the
package "paths" instead.

It matters less here because most users probably won't import it with a prefix
but in general, I like the idea of packages having names that are more like mass
nouns or adjectives, and less concrete singular nouns since the latter are more
likely to cause collisions.

Aside from that, I just skimmed this for general feedback. I'm not very familiar
with the resource stuff, so I don't have a lot of context. How would you feel
about getting Natalie to review this?

https://codereview.chromium.org/1387163002/diff/1/.travis.yml
File .travis.yml (right):

https://codereview.chromium.org/1387163002/diff/1/.travis.yml#newcode7
.travis.yml:7: sudo: false
This is the default behavior (for newer repositories), so you can omit this.

https://codereview.chromium.org/1387163002/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1387163002/diff/1/CHANGELOG.md#newcode3
CHANGELOG.md:3: ## 1.0.0
Bold move!

Are you really sure the API is stable and solid? Do we have real-world users of
it that we've gotten feedback on?

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md
File CONTRIBUTING.md (right):

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md#newcode4
CONTRIBUTING.md:4: ### Before you contribute
Markdown nit: can you add a blank line after this?

It isn't needed by all Markdown parsers, but some do. I also find it easier to
read in source with a blank line.

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:49: Resource resource(Uri uri, { ResourceLoader loader })
{
Can this have a verb phrase name? Maybe "loadResource" or "resolveResource"?

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:121: /// Implementation of [new Resoure] constructor.
"Resource".

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:123: /// Stores only a plain string, and
...?

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:214: throw "WHAT?";
?

[Lasse Reichstein Nielsen, 2015-10-08 17:20:54]

https://codereview.chromium.org/1387163002/diff/1/.travis.yml
File .travis.yml (right):

https://codereview.chromium.org/1387163002/diff/1/.travis.yml#newcode7
.travis.yml:7: sudo: false
Ack. I was copying files blindly from the "new repository" template. That may
need to be updated too.

https://codereview.chromium.org/1387163002/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1387163002/diff/1/CHANGELOG.md#newcode3
CHANGELOG.md:3: ## 1.0.0
If it changes, we go to 2.0.0.

It really, really irks me to have packages hoover in the 0.9.x+y ranges for a
long time. Release and iterate!

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md
File CONTRIBUTING.md (right):

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md#newcode4
CONTRIBUTING.md:4: ### Before you contribute
Sure. Again, taken verbatim from template.

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:49: Resource resource(Uri uri, { ResourceLoader loader })
{
createResource?

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:121: /// Implementation of [new Resoure] constructor.
On 2015/10/08 16:48:33, Bob Nystrom wrote:
> "Resource".

Acknowledged.

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:123: /// Stores only a plain string, and
Maybe it's all it does? :)
Will fix.

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:214: throw "WHAT?";
Whoops, debug code.
It did find the bug!

[Bob Nystrom, 2015-10-08 17:32:20]

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:49: Resource resource(Uri uri, { ResourceLoader loader })
{
On 2015/10/08 17:20:54, Lasse Reichstein Nielsen wrote:
> createResource?

"create" feels like the wrong word to me because the core idea with resources is
that they already exist. They are "premade" in some sense. Maybe just "get" or
"find"?

[Lasse Reichstein Nielsen, 2015-10-13 07:41:55]

Ping Florian

[floitsch, 2015-10-13 12:47:50]

LGTM (once Bob's comments have been addressed).

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md
File CONTRIBUTING.md (right):

https://codereview.chromium.org/1387163002/diff/1/CONTRIBUTING.md#newcode12
CONTRIBUTING.md:12: the CLA until after you've submitted your code for review
and a member has
I was not aware of this. Where does this come from? (Who wrote it?)

https://codereview.chromium.org/1387163002/diff/1/lib/src/loader.dart
File lib/src/loader.dart (right):

https://codereview.chromium.org/1387163002/diff/1/lib/src/loader.dart#newcode17
lib/src/loader.dart:17: /// Reads the file located by [uri] as a stream of
bytes.
I wouldn't use the term "file".

https://codereview.chromium.org/1387163002/diff/1/lib/src/loader.dart#newcode26
lib/src/loader.dart:26: /// If [encoding] is omittted, it defaults to UTF-8.
omitted

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:49: Resource resource(Uri uri, { ResourceLoader loader })
{
On 2015/10/08 17:32:20, Bob Nystrom wrote:
> On 2015/10/08 17:20:54, Lasse Reichstein Nielsen wrote:
> > createResource?
> 
> "create" feels like the wrong word to me because the core idea with resources
is
> that they already exist. They are "premade" in some sense. Maybe just "get" or
> "find"?

I would go with "get".

https://codereview.chromium.org/1387163002/diff/1/lib/src/resource.dart#newco...
lib/src/resource.dart:99: factory Resource.forUri(Uri uri, {ResourceLoader
loader}) =>
I think we often used "fromX" for this use-case.
"forUri" sounds fine to me, though.

[ahe, 2015-10-14 12:38:59]

https://codereview.chromium.org/1387163002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1387163002/diff/1/pubspec.yaml#newcode5
pubspec.yaml:5: homepage: https://github.com/dart-lang/resource
This URL is broken.

[ahe, 2015-10-14 12:39:29]

https://codereview.chromium.org/1387163002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/1387163002/diff/1/README.md#newcode9
README.md:9: [tracker]: https://github.com/dart-lang/resource/issues
This URL is broken.

[Lasse Reichstein Nielsen, 2015-10-14 12:43:28]

On 2015/10/14 12:39:29, ahe wrote:
> https://codereview.chromium.org/1387163002/diff/1/README.md#newcode9
> README.md:9: [tracker]: https://github.com/dart-lang/resource/issues
> This URL is broken.

Repository is now public.

[Lasse Reichstein Nielsen, 2015-10-27 08:35:15]

On 2015/10/14 12:43:28, Lasse Reichstein Nielsen wrote:
> Repository is now public.

Repository is now private again since it's not in a valid state (e.g., no
LICENSE file) until this CL lands.

[Lasse Reichstein Nielsen, 2016-01-14 10:43:18]

Updated, PTAL.

[floitsch, 2016-01-14 15:11:04]

LGTM.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/io.dart
File lib/src/io.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/io.dart#newcode71
lib/src/io.dart:71: Uint8Buffer buffer = new Uint8Buffer(length);
might as well make this a var.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart
File lib/src/loader.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart#new...
lib/src/loader.dart:14: /// An implementation decides which URI schemes it
supports.
Implementations of this interface decide which URI schemes they support.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart#new...
lib/src/loader.dart:23: /// (For example, file: URIs are not supported in the
browser).
You need a new line after * package.
Otherwise the remaining text is part of the last bullet point.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart#n...
lib/src/resource.dart:44: /// Read the resource content as a stream of bytes.
Reads

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart#n...
lib/src/resource.dart:47: /// Read the resource content as a single list of
bytes.
ditto.

[Lasse Reichstein Nielsen, 2016-01-15 09:32:24]

Description was changed from

==========
Add Resource package.
==========

to

==========
Add Resource package.

R=floitsch@google.com

Committed: 527b11d472abbdc4ee3d08749f5685e2265f3465
==========

[Lasse Reichstein Nielsen, 2016-01-15 09:32:25]

Committed patchset #5 (id:80001) manually as
527b11d472abbdc4ee3d08749f5685e2265f3465 (presubmit successful).

[Lasse Reichstein Nielsen, 2016-01-15 09:40:02]

https://codereview.chromium.org/1387163002/diff/60001/lib/src/io.dart
File lib/src/io.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/io.dart#newcode71
lib/src/io.dart:71: Uint8Buffer buffer = new Uint8Buffer(length);
On 2016/01/14 15:11:04, floitsch wrote:
> might as well make this a var.

Done.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart
File lib/src/loader.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart#new...
lib/src/loader.dart:14: /// An implementation decides which URI schemes it
supports.
On 2016/01/14 15:11:04, floitsch wrote:
> Implementations of this interface decide which URI schemes they support.

Done.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/loader.dart#new...
lib/src/loader.dart:23: /// (For example, file: URIs are not supported in the
browser).
On 2016/01/14 15:11:04, floitsch wrote:
> You need a new line after * package.
> Otherwise the remaining text is part of the last bullet point.

Done.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart
File lib/src/resource.dart (right):

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart#n...
lib/src/resource.dart:44: /// Read the resource content as a stream of bytes.
On 2016/01/14 15:11:04, floitsch wrote:
> Reads

Done.

https://codereview.chromium.org/1387163002/diff/60001/lib/src/resource.dart#n...
lib/src/resource.dart:47: /// Read the resource content as a single list of
bytes.
On 2016/01/14 15:11:04, floitsch wrote:
> ditto.

Done.