-
Notifications
You must be signed in to change notification settings - Fork 29.7k
adjust declared types to work with dart 2 typing at runtime #19007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
We'll also want to update to the (just recently published) latest version of |
mravn-google
left a comment
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.
LGTM
| List<Map<String, dynamic>> _getMaterialFonts(String fontSet) { | ||
| return _materialFontsManifest[fontSet]; | ||
| final List<dynamic> fontsList = _materialFontsManifest[fontSet]; | ||
| return fontsList?.map(castStringKeyedMap)?.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.
List.map should be given a type parameter per Flutter style guide:
return fontsList?.map<Map<String, dynamic>>(castStringKeyedMap)?.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.
done
| return _handlers[command](args); | ||
| throw 'command not understood: $name.$command'; | ||
| }).then<Null>((dynamic result) { | ||
| }).then<dynamic>((dynamic result) { |
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.
then<void>?
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 we want dynamic here, to match what we get from _handlers[command](args).
| }); | ||
|
|
||
| await app._runInZone(this, () async { | ||
| await app._runInZone<Null>(this, () async { |
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.
_runInZone<void>?
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.
Hmm, at runtime, this seemed to want a Null.
| final List<AnalysisError> errors = issueInfo['errors'] | ||
| final List<dynamic> errorsList = issueInfo['errors']; | ||
| final List<AnalysisError> errors = errorsList | ||
| .map(castStringKeyedMap) |
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.
The two map calls should have type parameters Map<String, dynamic> and AnalysisError, respectively.
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.
added
| pubspec._flutterDescriptor = flutterMap.cast<String, dynamic>(); | ||
| } else { | ||
| pubspec._flutterDescriptor = <String, dynamic>{}; | ||
| } |
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.
Maybe extract this pattern to a facility like castStringKeyedMap. We could let that method return an empty map on null input (but then we might also want to rename it toStringKeyedMap).
| final List<dynamic> fontList = _flutterDescriptor['fonts']; | ||
| return fontList == null | ||
| ? const <Map<String, dynamic>>[] | ||
| : fontList.map(castStringKeyedMap).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.
Type parameter on map...
This gets annoying. Perhaps we should have
Map<String, dynamic> toStringKeyedMap(dynamic json) {
if (json == null) {
return <String, dynamic>{};
} else {
final Map<dynamic, dynamic> map = json;
return map.cast<String, dynamic>();
}
}
List<Map<String, dynamic>> toStringKeyedMaps(dynamic json) {
if (json == null) {
return <Map<String, dynamic>>[];
} else {
final List<dynamic> list = json;
return list.map<Map<String, dynamic>>(toStringKeyedMap).toList();
}
}Alternative naming: toJsonObject/toJsonArray.
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.
The empty map and list could be const, but then the semantics gets complicated: toStringKeyedMap(null) is immutable, whereas toStringKeyedMap({}) is not. If we do use const we should return immutable objects in all cases.
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.
Let me look into creating a toStringKeyedMaps() 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.
@mravn-google, I may save that for a follow-up PR (toStringKeyedMaps).
| if (assets == null) { | ||
| return const <Uri>[]; | ||
| } | ||
| return assets.cast<String>().map(Uri.encodeFull)?.map(Uri.parse)?.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.
Type parameter on map, twice.
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
| for (String deviceCategory in devicesSection.keys) { | ||
| final List<dynamic> devicesData = devicesSection[deviceCategory]; | ||
| for (Map<String, dynamic> data in devicesData.map(_castStringKeyedMap)) { | ||
| for (Map<String, dynamic> data in devicesData.map(castStringKeyedMap)) { |
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.
Type parameter on map.
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
| ) { | ||
| return Future.any(<Future<Map<String, dynamic>>>[ | ||
| _peer.sendRequest(method, params), | ||
| _peer.sendRequest(method, params).then(castStringKeyedMap), |
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.
Type parameter on then.
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.
added
| bool get isClosed => _peer.isClosed; | ||
| Future<Null> get done => _peer.done; | ||
|
|
||
| Future<Null> get done async { |
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.
Future<void>?
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 really don't know if we're returning null or void here.
Fix several issues when running flutter_tools in --preview-dart-2:
cast()data structures parsed from json so that the types at runtime match what we expectWe'll need to change the flutter_tools tests over to running in
--preview-dart-2in order to keep the code dart 2 safe, but we're not quite ready to do that (our test suite is reparsed for each individual test - the test suite takes 40-50 minutes to run).