-
Notifications
You must be signed in to change notification settings - Fork 23
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
chore: refactor code style #186
Conversation
@@ -23,8 +23,7 @@ enum LeakType { | |||
/// Disposed and garbage collected later than expected. | |||
gcedLate; | |||
|
|||
static LeakType byName(String name) => | |||
LeakType.values.firstWhere((e) => e.name == name); | |||
static LeakType byName(String name) => LeakType.values.byName(name); | |||
} |
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.
Good suggestion.
} | ||
theContext.addAll(addedContext); | ||
context ??= {}; | ||
context!.addAll(addedContext); | ||
} | ||
|
||
LeakReport toLeakReport() => LeakReport( |
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 intent of the original code to avoid !
and to use null-safety.
The new code has '!' and thus moves null-safety validation from compiler to developer, making it error prone.
Can you revert changes in this files?
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.
sure.
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 used the ! syntax because the context must not be null in this case.
How about this change? I will submit it if it's acceptable.
void setContext(String key, Object value) {
context ??= {};
context?[key] = value;
}
void mergeContext(Map<String, dynamic>? addedContext) {
if (addedContext == null) return;
context ??= {};
context?.addAll(addedContext);
}
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.
Looks good. Thank you.
Merged the change. Feel free to create new PR with others. |
improve the code style and make the code more clear
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.