Binary assets and more unit tests for Asset.

Binary assets and more unit tests for Asset.

R=nweiz@google.com

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

[nweiz, 2013-07-08 22:50:46]

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:78: buffer.write(digits[byte % 16]);
Add an intToHex function in utils.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:82: if (_contents.length > 10) {
It's weird that an 11-byte buffer is abbreviated to something longer than it
would have been un-abbreviated. This abbreviation scheme only starts to save
space with _contents.length > 12.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.dart
File pkg/barback/test/asset_test.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:38: new
File(utf32FilePath).writeAsStringSync("çøñ†éℵ™");
It doesn't look like you're actually writing this as UTF-32.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:66: 
Extra newline.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:114: });
Add a TODO to test that this supports other encodings.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:141: completion(equals("çøñ†éℵ™")));
Since it looks like utf32FilePath isn't being written as UTF-32, how is this
test passing?

[Bob Nystrom, 2013-07-08 23:26:04]

Thanks!

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:78: buffer.write(digits[byte % 16]);
On 2013/07/08 22:50:46, nweiz wrote:
> Add an intToHex function in utils.

Done.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:82: if (_contents.length > 10) {
On 2013/07/08 22:50:46, nweiz wrote:
> It's weird that an 11-byte buffer is abbreviated to something longer than it
> would have been un-abbreviated. This abbreviation scheme only starts to save
> space with _contents.length > 12.

I suppose so, but I think this is tolerable balance between user-friendly output
and simple code. toString() is already the longest method in the class. :)

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.dart
File pkg/barback/test/asset_test.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:38: new
File(utf32FilePath).writeAsStringSync("çøñ†éℵ™");
On 2013/07/08 22:50:46, nweiz wrote:
> It doesn't look like you're actually writing this as UTF-32.

Eek crap. Copy/paste bug.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:66: 
On 2013/07/08 22:50:46, nweiz wrote:
> Extra newline.

Done.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:114: });
On 2013/07/08 22:50:46, nweiz wrote:
> Add a TODO to test that this supports other encodings.

Done.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:141: completion(equals("çøñ†éℵ™")));
On 2013/07/08 22:50:46, nweiz wrote:
> Since it looks like utf32FilePath isn't being written as UTF-32, how is this
> test passing?

Two bugs collide! My bug outputs the file in UTF-8. Then, it turns out
Encoding.fromName() doesn't actually support any of the UTF encodings. (I know,
right.) So it returns null instead of throwing an exception. (I know, right.)
That gets passed to readAsString() so it uses the default encoding... UTF-8.

Filed a bug, commented this test out, and added a TODO.

[nweiz, 2013-07-09 02:00:01]

A couple more suggestions, otherwise lgtm.

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:82: if (_contents.length > 10) {
On 2013/07/08 23:26:04, Bob Nystrom wrote:
> On 2013/07/08 22:50:46, nweiz wrote:
> > It's weird that an 11-byte buffer is abbreviated to something longer than it
> > would have been un-abbreviated. This abbreviation scheme only starts to save
> > space with _contents.length > 12.
> 
> I suppose so, but I think this is tolerable balance between user-friendly
output
> and simple code. toString() is already the longest method in the class. :)

All you have to do is change "> 10" to "> 11".

https://codereview.chromium.org/18854007/diff/5/pkg/barback/lib/src/utils.dart
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/18854007/diff/5/pkg/barback/lib/src/utils.dar...
pkg/barback/lib/src/utils.dart:10: String byteToHex(int byte) {
assert(byte >= 0 && byte < 256)

https://codereview.chromium.org/18854007/diff/5/pkg/barback/test/asset_test.dart
File pkg/barback/test/asset_test.dart (right):

https://codereview.chromium.org/18854007/diff/5/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:139: // TODO(rnystrom): Disabled until #11744
is fixed.
I'd rather just remove this entirely. I'm not a fan of commented-out code.

[Bob Nystrom, 2013-07-09 16:25:22]

Committed patchset #3 manually as r24848 (presubmit successful).

[Bob Nystrom, 2013-07-09 16:26:03]

Thanks!

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18854007/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:82: if (_contents.length > 10) {
On 2013/07/09 02:00:01, nweiz wrote:
> On 2013/07/08 23:26:04, Bob Nystrom wrote:
> > On 2013/07/08 22:50:46, nweiz wrote:
> > > It's weird that an 11-byte buffer is abbreviated to something longer than
it
> > > would have been un-abbreviated. This abbreviation scheme only starts to
save
> > > space with _contents.length > 12.
> > 
> > I suppose so, but I think this is tolerable balance between user-friendly
> output
> > and simple code. toString() is already the longest method in the class. :)
> 
> All you have to do is change "> 10" to "> 11".

Done.

https://codereview.chromium.org/18854007/diff/5/pkg/barback/lib/src/utils.dart
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/18854007/diff/5/pkg/barback/lib/src/utils.dar...
pkg/barback/lib/src/utils.dart:10: String byteToHex(int byte) {
On 2013/07/09 02:00:01, nweiz wrote:
> assert(byte >= 0 && byte < 256)

Done.

https://codereview.chromium.org/18854007/diff/5/pkg/barback/test/asset_test.dart
File pkg/barback/test/asset_test.dart (right):

https://codereview.chromium.org/18854007/diff/5/pkg/barback/test/asset_test.d...
pkg/barback/test/asset_test.dart:139: // TODO(rnystrom): Disabled until #11744
is fixed.
On 2013/07/09 02:00:01, nweiz wrote:
> I'd rather just remove this entirely. I'm not a fan of commented-out code.

Done.