pkg/serialization: cleanup

pkg/serialization: cleanup

R=alanknight@google.com

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

[kevmoo-old, 2013-06-21 22:28:23]

While investigating a bug, I did a bit of cleanup

[Alan Knight, 2013-06-21 22:47:46]

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:550: void clear() => _throw();
You can't declare things void and use the => syntax. Which I find annoying. See
https://code.google.com/p/dart/issues/detail?id=11291

In this case it's rather moot that the function would throw in checked mode,
because it will throw anyway. Although since _throw is also void, presumably if
used in an expression its return value is null, which would then get past
checked mode. But if we're to consider void incompatible with lambda syntax we
probably shouldn't do it.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:568: int get length =>
_raw.length;
Nice to be able to get rid of all that boilerplate. Thanks.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:16: void main() {
You really like those type signatures, don't you :-)

I'm skeptical that these are providing a lot of value, but I don't think it
hurts, so fine.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:557: return
port.call(output).then(verify);
Does that actually work? Don't you need some level of expectAsync in there
somewhere to make sure that verify is called before the test framework considers
everything done?

[kevmoo-old, 2013-06-21 22:57:31]

Replies

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:550: void clear() => _throw();
On 2013/06/21 22:47:46, Alan Knight wrote:
> You can't declare things void and use the => syntax. Which I find annoying.
See
> https://code.google.com/p/dart/issues/detail?id=11291
> 
> In this case it's rather moot that the function would throw in checked mode,
> because it will throw anyway. Although since _throw is also void, presumably
if
> used in an expression its return value is null, which would then get past
> checked mode. But if we're to consider void incompatible with lambda syntax we
> probably shouldn't do it. 

It's more about incompatible return types.

If _throw returned int, this would blow up.

As is, it works fine...even in checked mode.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:568: int get length =>
_raw.length;
On 2013/06/21 22:47:46, Alan Knight wrote:
> Nice to be able to get rid of all that boilerplate. Thanks.

Yup yup

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:16: void main() {
On 2013/06/21 22:47:46, Alan Knight wrote:
> You really like those type signatures, don't you :-)
> 
> I'm skeptical that these are providing a lot of value, but I don't think it
> hurts, so fine.

Mostly about readability. Being explicit about what a method returns (even if
it's nothing) I find really nice.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:557: return
port.call(output).then(verify);
On 2013/06/21 22:47:46, Alan Knight wrote:
> Does that actually work? Don't you need some level of expectAsync in there
> somewhere to make sure that verify is called before the test framework
considers
> everything done?

If a test returns a future, then the test system waits for the future to
complete (or error out).

Passing verify in to evaluate the result ends up super clean.

[Alan Knight, 2013-06-21 23:15:20]

LGTM.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
File pkg/serialization/lib/src/serialization_rule.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/lib/src/ser...
pkg/serialization/lib/src/serialization_rule.dart:550: void clear() => _throw();
On 2013/06/21 22:57:32, kevmoo wrote:
> On 2013/06/21 22:47:46, Alan Knight wrote:
> > You can't declare things void and use the => syntax. Which I find annoying.
> See
> > https://code.google.com/p/dart/issues/detail?id=11291
> > 
> > In this case it's rather moot that the function would throw in checked mode,
> > because it will throw anyway. Although since _throw is also void, presumably
> if
> > used in an expression its return value is null, which would then get past
> > checked mode. But if we're to consider void incompatible with lambda syntax
we
> > probably shouldn't do it. 
> 
> It's more about incompatible return types.
> 
> If _throw returned int, this would blow up.
> 
> As is, it works fine...even in checked mode.

Since _throw never returns, it's moot anyway. But the feedback I got offline was
that void functions should be considered incompatible with lambda syntax. To me
void means "doesn't return a value", and the fact that null is considered
compatible with that is a little odd. But it's moot anyway.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
File pkg/serialization/test/serialization_test.dart (right):

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:16: void main() {
On 2013/06/21 22:57:32, kevmoo wrote:
> On 2013/06/21 22:47:46, Alan Knight wrote:
> > You really like those type signatures, don't you :-)
> > 
> > I'm skeptical that these are providing a lot of value, but I don't think it
> > hurts, so fine.
> 
> Mostly about readability. Being explicit about what a method returns (even if
> it's nothing) I find really nice.

I, on the other hand, find that it adds noise and makes it less readable. But
it's not a big deal either way.

https://codereview.chromium.org/17379025/diff/1/pkg/serialization/test/serial...
pkg/serialization/test/serialization_test.dart:557: return
port.call(output).then(verify);
On 2013/06/21 22:57:32, kevmoo wrote:
> On 2013/06/21 22:47:46, Alan Knight wrote:
> > Does that actually work? Don't you need some level of expectAsync in there
> > somewhere to make sure that verify is called before the test framework
> considers
> > everything done?
> 
> If a test returns a future, then the test system waits for the future to
> complete (or error out).
> 
> Passing verify in to evaluate the result ends up super clean.

OK, I'll take your word for it.

[kevmoo-old, 2013-06-21 23:24:30]

Committed patchset #2 manually as r24306 (presubmit successful).