Adding package:smoke. This CL includes the intial APIs, a mirror-based

Adding package:smoke. This CL includes the intial APIs, a mirror-based
implementation, a static-based implementation (meant to be used with codegen one
day), and tests.

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-02-20 04:54:36]

https://codereview.chromium.org/169913004/diff/30001/pkg/smoke/lib/smoke.dart
File pkg/smoke/lib/smoke.dart (right):

https://codereview.chromium.org/169913004/diff/30001/pkg/smoke/lib/smoke.dart...
pkg/smoke/lib/smoke.dart:155: abstract class ObjectAccessorService {
FYI - I am not convinced about the right design principle here.

I went with this 3 service distinction initially to make it clear the scope of
each service, especially thinking that in the future we could have more services
added to deal with expressions. I'm not sure how much this buy us though.

An alternative here is to blend all 3 services into a single class that
implements all of smoke.

Either way, the API surface of smoke is mainly through the top-level methods, so
we can iterate on this over time.

Thoughts?

[Jennifer Messerly, 2014-02-20 21:24:47]

LGTM. Where G stands for Grawesome!

https://codereview.chromium.org/169913004/diff/30001/pkg/smoke/lib/smoke.dart
File pkg/smoke/lib/smoke.dart (right):

https://codereview.chromium.org/169913004/diff/30001/pkg/smoke/lib/smoke.dart...
pkg/smoke/lib/smoke.dart:155: abstract class ObjectAccessorService {
On 2014/02/20 04:54:36, Siggi Cherem (dart-lang) wrote:
> FYI - I am not convinced about the right design principle here.
> 
> I went with this 3 service distinction initially to make it clear the scope of
> each service, especially thinking that in the future we could have more
services
> added to deal with expressions. I'm not sure how much this buy us though.
> 
> An alternative here is to blend all 3 services into a single class that
> implements all of smoke.
> 
> Either way, the API surface of smoke is mainly through the top-level methods,
so
> we can iterate on this over time.
> 
> Thoughts?

seems reasonable to me. I don't have a strong feeling, but the break up felt
pretty natural reading the impls, so maybe keep? Always easier to merge later
than it will be to split things up, since things tend to naturally get tangled
:)

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/README.md
File pkg/smoke/README.md (right):

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/README.md#newc...
pkg/smoke/README.md:16: `MirrorsUsed` annotation). Instead, this package
provides the building blocks
perhaps link to api docs, e.g.

[MirrorsUsed](https://api.dartlang.org/apidocs/channels/stable/#dart-mirrors.MirrorsUsed)

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.dart
File pkg/smoke/lib/mirrors.dart (right):

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.da...
pkg/smoke/lib/mirrors.dart:208: //
https://code.google.com/p/dart/issues/detail?id=15573
fyi -- this one has been fixed, I think we can now just catch UnsupportedError.

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.da...
pkg/smoke/lib/mirrors.dart:233: var setterName = new
Symbol('${MirrorSystem.getName(getter.simpleName)}=');
use _setterName here?

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/src/defau...
File pkg/smoke/lib/src/default_transformer.dart (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/src/defau...
pkg/smoke/lib/src/default_transformer.dart:32: .replaceAll("import
'package:smoke/mirrors.dart'';", '')));
is there an extra ' here? mirrors.dart''

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.dart
File pkg/smoke/lib/static.dart (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:12: import 'dart:math' as math;
super duper trivial nit, feel free to ignore: I usually put imports in dart:,
pkg:, relative path order.

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:87: _logger.warning('we tried to adjust the arguments
for calling "$name"'
hmm, this might be a case where try+catch+rethrow would work better. if someone
doesn't have the logging turned on they won't see this (very helpful!) message
but will instead just see NoSuchMethodError from the Function.apply

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:112: return (decl != null && !decl.isStatic);
fyi ... parens are not needed here or in other methods below

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/pubspec.yaml
File pkg/smoke/pubspec.yaml (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/pubspec.yaml#...
pkg/smoke/pubspec.yaml:11: transformers: [smoke/src/default_transformer]
qq: do people need to run this outside of this pkg? I guess not since you put it
in src/ :)

[Siggi Cherem (dart-lang), 2014-02-20 22:19:47]

Thanks John!

QQ: about logging vs printing. PTAL if you agree with how now I'm printing by
default when we catch the invocation error. Should we also just use print
instead of logger for the other places in that file where I'm using logger?

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/README.md
File pkg/smoke/README.md (right):

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/README.md#newc...
pkg/smoke/README.md:16: `MirrorsUsed` annotation). Instead, this package
provides the building blocks
On 2014/02/20 21:24:47, John Messerly wrote:
> perhaps link to api docs, e.g.
> 
>
[MirrorsUsed](https://api.dartlang.org/apidocs/channels/stable/#dart-mirrors.MirrorsUsed)

done.

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.dart
File pkg/smoke/lib/mirrors.dart (right):

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.da...
pkg/smoke/lib/mirrors.dart:208: //
https://code.google.com/p/dart/issues/detail?id=15573
On 2014/02/20 21:24:47, John Messerly wrote:
> fyi -- this one has been fixed, I think we can now just catch
UnsupportedError.

cool. good to know.

https://codereview.chromium.org/169913004/diff/70001/pkg/smoke/lib/mirrors.da...
pkg/smoke/lib/mirrors.dart:233: var setterName = new
Symbol('${MirrorSystem.getName(getter.simpleName)}=');
On 2014/02/20 21:24:47, John Messerly wrote:
> use _setterName here?

Done.

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/src/defau...
File pkg/smoke/lib/src/default_transformer.dart (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/src/defau...
pkg/smoke/lib/src/default_transformer.dart:32: .replaceAll("import
'package:smoke/mirrors.dart'';", '')));
On 2014/02/20 21:24:47, John Messerly wrote:
> is there an extra ' here? mirrors.dart''

good catch! I had renamed the library and when I fixed the import here I seem to
have pasted the new name with the quote, doh! Definitely need to get some tests
for this soon.

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.dart
File pkg/smoke/lib/static.dart (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:12: import 'dart:math' as math;
On 2014/02/20 21:24:47, John Messerly wrote:
> super duper trivial nit, feel free to ignore: I usually put imports in dart:,
> pkg:, relative path order.

oops, me too, thanks!

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:87: _logger.warning('we tried to adjust the arguments
for calling "$name"'
On 2014/02/20 21:24:47, John Messerly wrote:
> hmm, this might be a case where try+catch+rethrow would work better. if
someone
> doesn't have the logging turned on they won't see this (very helpful!) message
> but will instead just see NoSuchMethodError from the Function.apply

good call. Done. Now we only show the warning/error when the exception is
thrown.

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:112: return (decl != null && !decl.isStatic);
On 2014/02/20 21:24:47, John Messerly wrote:
> fyi ... parens are not needed here or in other methods below

Done.

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/pubspec.yaml
File pkg/smoke/pubspec.yaml (right):

https://codereview.chromium.org/169913004/diff/120002/pkg/smoke/pubspec.yaml#...
pkg/smoke/pubspec.yaml:11: transformers: [smoke/src/default_transformer]
On 2014/02/20 21:24:47, John Messerly wrote:
> qq: do people need to run this outside of this pkg? I guess not since you put
it
> in src/ :)

exactly, no need to run this in other packages. They will need a different
transformer to generate code, but this one particularly will be hidden.

With this hidden transformer we end up having everyone using the mirror
implementation when developing directly in Dartium by default, but mirrors are
removed behind the scenes whenever you use pub-serve or pub-build.

[Jennifer Messerly, 2014-02-20 23:24:36]

yeah, hmm, that's a tricky one. I wish logged warnings showed up by default in
the browser console. Maybe that's the underlying issue here. It's a shame in a
library like this you can't print warnings that people will see, because you
don't was to depend on dart:html ...

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.dart
File pkg/smoke/lib/static.dart (right):

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:107: if (tentativeError != null)
print(tentativeError);
+1, this is nice!

one other thing this makes me think of ... you could actually wrap the exception
+ stack trace here and throw your new exception with more info. That would also
handle the case like someone who is catching & logging exceptions but not print
statements. I dunno if it is worth it tho, or if there is a good way to do that
without losing the stack trace.

[Siggi Cherem (dart-lang), 2014-02-21 01:51:24]

thanks!

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.dart
File pkg/smoke/lib/static.dart (right):

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:107: if (tentativeError != null)
print(tentativeError);
On 2014/02/20 23:24:37, John Messerly wrote:
> +1, this is nice!
> 
> one other thing this makes me think of ... you could actually wrap the
exception
> + stack trace here and throw your new exception with more info. That would
also
> handle the case like someone who is catching & logging exceptions but not
print
> statements. I dunno if it is worth it tho, or if there is a good way to do
that
> without losing the stack trace.

mmm, tricky. good question. I'm inclined for now to leave it as it is. I'll add
a TODO to revisit this as we get more experience using this.

[Jennifer Messerly, 2014-02-21 01:52:14]

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.dart
File pkg/smoke/lib/static.dart (right):

https://codereview.chromium.org/169913004/diff/250001/pkg/smoke/lib/static.da...
pkg/smoke/lib/static.dart:107: if (tentativeError != null)
print(tentativeError);
On 2014/02/21 01:51:24, Siggi Cherem (dart-lang) wrote:
> On 2014/02/20 23:24:37, John Messerly wrote:
> > +1, this is nice!
> > 
> > one other thing this makes me think of ... you could actually wrap the
> exception
> > + stack trace here and throw your new exception with more info. That would
> also
> > handle the case like someone who is catching & logging exceptions but not
> print
> > statements. I dunno if it is worth it tho, or if there is a good way to do
> that
> > without losing the stack trace.
> 
> mmm, tricky. good question. I'm inclined for now to leave it as it is. I'll
add
> a TODO to revisit this as we get more experience using this.

SGTM

[Siggi Cherem (dart-lang), 2014-02-21 03:11:00]

Committed patchset #5 manually as r32878 (presubmit successful).