Adapting observe to use smoke (this is built on top of the previous change that

Adapting observe to use smoke (this is built on top of the previous change that
adds the smoke package)

R=jmesserly@google.com

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

[kasperl, 2014-02-20 10:16:18]

https://codereview.chromium.org/173473002/diff/1/pkg/observe/lib/src/observab...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/173473002/diff/1/pkg/observe/lib/src/observab...
pkg/observe/lib/src/observable.dart:197: _unused() => unused.MirrorSystem; // to
prevent analyzer warnings
File analyzer bug for this? The import is clearly used because we get
MirrorsUsed from it.

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

https://codereview.chromium.org/173473002/diff/1/pkg/observe/lib/src/observab...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/173473002/diff/1/pkg/observe/lib/src/observab...
pkg/observe/lib/src/observable.dart:197: _unused() => unused.MirrorSystem; // to
prevent analyzer warnings
On 2014/02/20 10:16:18, kasperl wrote:
> File analyzer bug for this? The import is clearly used because we get
> MirrorsUsed from it.

Thanks for pointing this one out. It seems that this is now handled correctly by
the analyzer. I was probably confused because in another place I had to include
dart:mirrors without any @MirrorsUsed annotation (to preserve everything), so I
had to add this there as well.

[Jennifer Messerly, 2014-02-20 21:46:39]

https://codereview.chromium.org/173473002/diff/60001/pkg/observe/lib/src/obse...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/173473002/diff/60001/pkg/observe/lib/src/obse...
pkg/observe/lib/src/observable.dart:18: import 'dart:mirrors' as unused;
import 'dart:mirrors' show MirrorsUsed?

https://codereview.chromium.org/173473002/diff/60001/pkg/observe/lib/src/obse...
pkg/observe/lib/src/observable.dart:87: var queryOptions = new
smoke.QueryOptions(includeInherited: true,
hmmm, yeah, it's pretty important not to find getters. They could execute code
(see comment below). Is it hard to add this to pkg:smoke? 

or can you use if (!decl.isProperty) to filter it inside the for loop?

[Siggi Cherem (dart-lang), 2014-02-21 03:50:27]

Thanks John, PTAL

https://codereview.chromium.org/173473002/diff/60001/pkg/observe/lib/src/obse...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/173473002/diff/60001/pkg/observe/lib/src/obse...
pkg/observe/lib/src/observable.dart:87: var queryOptions = new
smoke.QueryOptions(includeInherited: true,
On 2014/02/20 21:46:40, John Messerly wrote:
> hmmm, yeah, it's pretty important not to find getters. They could execute code
> (see comment below). Is it hard to add this to pkg:smoke? 
> 
> or can you use if (!decl.isProperty) to filter it inside the for loop?

yeah, I had a feeling this was going to be necessary.

I included in this CL a change in smoke to be able to distinguish isProperty vs
isField and a way to filter it in the query.

[Siggi Cherem (dart-lang), 2014-02-21 18:02:58]

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

https://codereview.chromium.org/173473002/diff/130001/pkg/smoke/lib/smoke.dar...
pkg/smoke/lib/smoke.dart:130: /// Whether the symbol is a getter/setter and not
a field.
I was thinking more about it this morning, and I'm not convinced about making
isProperty false for fields. Some people might find that surprising or
unintuitive.

Alternatively we could do the following:

in declaration: isField implies isProperty
in query options:
  - includeProperties: query for both fields and properties
  - excludeGetters:  exclude getters from the property results

Thoughts?

[Jennifer Messerly, 2014-02-21 23:29:11]

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

https://codereview.chromium.org/173473002/diff/130001/pkg/smoke/lib/smoke.dar...
pkg/smoke/lib/smoke.dart:130: /// Whether the symbol is a getter/setter and not
a field.
On 2014/02/21 18:02:58, Siggi Cherem (dart-lang) wrote:
> I was thinking more about it this morning, and I'm not convinced about making
> isProperty false for fields. Some people might find that surprising or
> unintuitive.
> 
> Alternatively we could do the following:
> 
> in declaration: isField implies isProperty
> in query options:
>   - includeProperties: query for both fields and properties
>   - excludeGetters:  exclude getters from the property results
> 
> Thoughts?

Tricky. What you have right now is fairly consistent with mirrors, which doesn't
expose the "induced getter/setter" for fields, so on that count it's okay. In
mirrors I think it works okay to expose closer to what the source code says,
since sometimes that distinction matters (as it does for Observable).

excludeGetters seems confusing -- why getters and not setters?

but yeah, I can also see "isProperty" including fields. The way the
"DeclarationKind" enum is set up certainly would make me lean towards having
them separate.

https://codereview.chromium.org/173473002/diff/130001/pkg/smoke/lib/smoke.dar...
pkg/smoke/lib/smoke.dart:167: class DeclarationKind {
if _kind is private, then DeclarationKind should be too, IMO. Or alternatively
expose Declaration.kind

[Jennifer Messerly, 2014-02-21 23:29:36]

also -- LGTM

[Siggi Cherem (dart-lang), 2014-02-22 01:13:27]

thx

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

https://codereview.chromium.org/173473002/diff/130001/pkg/smoke/lib/smoke.dar...
pkg/smoke/lib/smoke.dart:130: /// Whether the symbol is a getter/setter and not
a field.
On 2014/02/21 23:29:11, John Messerly wrote:
> On 2014/02/21 18:02:58, Siggi Cherem (dart-lang) wrote:
> > I was thinking more about it this morning, and I'm not convinced about
making
> > isProperty false for fields. Some people might find that surprising or
> > unintuitive.
> > 
> > Alternatively we could do the following:
> > 
> > in declaration: isField implies isProperty
> > in query options:
> >   - includeProperties: query for both fields and properties
> >   - excludeGetters:  exclude getters from the property results
> > 
> > Thoughts?
> 
> Tricky. What you have right now is fairly consistent with mirrors, which
doesn't
> expose the "induced getter/setter" for fields, so on that count it's okay. In
> mirrors I think it works okay to expose closer to what the source code says,
> since sometimes that distinction matters (as it does for Observable).
> 
> excludeGetters seems confusing -- why getters and not setters?
> 
> but yeah, I can also see "isProperty" including fields. The way the
> "DeclarationKind" enum is set up certainly would make me lean towards having
> them separate.

ok, thanks - I went with keeping them separate and adjusted the comments to be
more explicit about it.

https://codereview.chromium.org/173473002/diff/130001/pkg/smoke/lib/smoke.dar...
pkg/smoke/lib/smoke.dart:167: class DeclarationKind {
On 2014/02/21 23:29:11, John Messerly wrote:
> if _kind is private, then DeclarationKind should be too, IMO. Or alternatively
> expose Declaration.kind

yeah, I was ambivalent about it. I decided to expose it in Declaration.kind in
the end. Constructing the declaration (see static_test.dart) is much nicer if
this is exposed.

[Siggi Cherem (dart-lang), 2014-02-24 23:45:11]

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