-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
7307081
to
6572c36
Compare
a50a051
to
5755e48
Compare
const _InitMethod(); | ||
|
||
@override | ||
initialize(_ZeroArg method) => method(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - if we change later StaticInitializer.initialize to return a future, we'd need to change this to not use =>
, but return null instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, for exactly that reason I think we should leave it as dynamic
Super cool! |
5755e48
to
2de31fb
Compare
ok updated everything and ran the formatter, ready for another look! |
…sses, and alphabetical within each category.
// The primary function in this class, invoke it to crawl and call all the | ||
// annotations. | ||
Future run() { | ||
// Parse everything into the two queues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the queues are gone, I think we can delete this and the following comment. It's pretty self explanatory now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
final InitializerFilter customFilter; | ||
|
||
// All the libraries we have seen so far. | ||
final Set<LibraryMirror> _librariesSeen = new Set<LibraryMirror>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that we might need to move this down and make it an argument of _readLibraryDeclarations below and not a field anymore. This is for the same reason we do the caching of annotations-found for each initializer: when we process initializers in phases and someone runs init.run
a second time, we should revisit all libraries to see if there were other initializers we should add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya true, this isn't an issue right now because I only ever call run() on one of these crawlers once, but since the class itself doesn't enforce that it could cause bugs later on.
updated, see note about sorting libraries by uri though, we should talk at some point today about that |
} else { | ||
var superMetas = declaration.superclass.metadata | ||
.where((m) => _filterMetadata(declaration.superclass, m)) | ||
.toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should additionally filter initializers that we have previously found, otherwise we might report the message unnecessarily on an example like this:
import 'a.dart';
@init
class B extends A {}
a.dart:
@init
class A {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(to clarify - the current check seems to be finding the super initializer, but is not excluding those that don't create a cycle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does cover this inside _filterMetadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nice, sorry, I totally missed that. Thanks for the clarification
ok, I think I just finished the second pass :) |
df450a6
to
2e33a7d
Compare
ok, ready for another look. I also added in the stuff we talked about with regards to ordering file imports by their relative uri to more closely match what the user typed. |
/// | ||
/// Call [run] from your main and this will print 'Foo says `hello world!`' | ||
/// | ||
abstract class StaticInitializer<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after our discussion earlier, should we rename this to just Initializer
? If so, should we rename initialize
as run
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will leave that rename for a followup cl
LGTM! Besides the comment about clustering package before file urls, I think we are good! |
This deviates from the design doc somewhat, differences are listed below:
run
,typeFilter
andcustomFilter
, instead of justfilter
. ThetypeFilter
is analogous to thefilter
described in the design doc, andcustomFilter
accepts a function with this signaturebool (StaticInitializer annotation)
.