-
Notifications
You must be signed in to change notification settings - Fork 27.8k
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
Fix error in type cast. #97778
Fix error in type cast. #97778
Conversation
@@ -993,7 +993,7 @@ class DebuggingOptions { | |||
debuggingEnabled: (json['debuggingEnabled'] as bool?)!, | |||
startPaused: (json['startPaused'] as bool?)!, | |||
dartFlags: (json['dartFlags'] as String?)!, | |||
dartEntrypointArgs: ((json['dartEntrypointArgs'] as List<String>?)?.cast<String>())!, | |||
dartEntrypointArgs: ((json['dartEntrypointArgs'] as List<dynamic>?)?.cast<String>())!, |
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'm a little confused as to this fix:
Casting to List is an error when the original list is List.
even after this change, the object could still be a list. It doesn't seem like calling list.cast<T>()
where list is already a List<T>
does not seem to be an error. Also, the test you added still passes without this fix.
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.
Github removed everything between <>
in my original comment, I meant to say "casting to List<String>
is an error when the original list is List<dynamic>
."
Updated the original description.
And I missed a step in the test, I've updated the test to convert it to JSON string, and parse the JSON into a Map<String, Object>
, and then run DebuggingOptions.fromJson
.
The object returned from json.decode
is a List<dynamic>
, and it is an error to cast that to List<String>
directly. It should be casted by using (list as List<dynamic>).cast<String>()
PTAL, thanks
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.
Ahh, ok, that makes sense 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.
LGTM
This pull request is not suitable for automatic merging in its current state.
|
Casting to
List<String>
is an error when the original list isList<dynamic>
.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.