-
Notifications
You must be signed in to change notification settings - Fork 356
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
Only omit 5 deprecation warnings per feature by default #1327
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// Copyright 2018 Google Inc. Use of this source code is governed by an | ||
// MIT-style license that can be found in the LICENSE file or at | ||
// https://opensource.org/licenses/MIT. | ||
|
||
import 'package:collection/collection.dart'; | ||
import 'package:source_span/source_span.dart'; | ||
import 'package:stack_trace/stack_trace.dart'; | ||
|
||
import '../logger.dart'; | ||
|
||
/// The maximum number of repetitions of the same warning [TerseLogger] will | ||
/// emit before hiding the rest. | ||
const _maxRepetitions = 5; | ||
|
||
/// A logger that wraps an inner logger to omit repeated deprecation warnings. | ||
/// | ||
/// A warning is considered "repeated" if the first paragraph is the same as | ||
/// another warning that's already been emitted. | ||
class TerseLogger implements Logger { | ||
/// A map from the first paragraph of a warning to the number of times this | ||
/// logger has emitted a warning with that line. | ||
final _warningCounts = <String, int>{}; | ||
|
||
final Logger _inner; | ||
|
||
TerseLogger(this._inner); | ||
|
||
void warn(String message, | ||
{FileSpan? span, Trace? trace, bool deprecation = false}) { | ||
if (deprecation) { | ||
var firstParagraph = message.split("\n\n").first; | ||
var count = _warningCounts[firstParagraph] = | ||
(_warningCounts[firstParagraph] ?? 0) + 1; | ||
if (count > _maxRepetitions) return; | ||
} | ||
|
||
_inner.warn(message, span: span, trace: trace, deprecation: deprecation); | ||
} | ||
|
||
void debug(String message, SourceSpan span) => _inner.debug(message, span); | ||
|
||
/// Prints a warning indicating the number of deprecation warnings that were | ||
/// omitted. | ||
/// | ||
/// The [node] flag indicates whether this is running in Node.js mode, in | ||
/// which case it doesn't mention "verbose mode" because the Node API doesn't | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Node API takes an option object. Adding support for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but adding a new feature (even a small one) to the Node.js API is a much more involved process. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one seems easy to add: #1332 |
||
/// support that. | ||
void summarize({required bool node}) { | ||
var total = _warningCounts.values | ||
.where((count) => count > _maxRepetitions) | ||
.map((count) => count - _maxRepetitions) | ||
.sum; | ||
if (total > 0) { | ||
_inner.warn("$total repetitive deprecation warnings omitted." + | ||
(node ? "" : "\nRun in verbose mode to see all warnings.")); | ||
} | ||
} | ||
} |
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.
My only concern is that we're now assigning special semantics to
\n\n
in a warning, which could cause confusion. But, I'm not sure if any other heuristic would make sense here.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 could theoretically try to pass in some sort of side-channel "deprecation type" identifier, maybe through Zones, but I think that would be more complicated than would be necessary. Given that we control both this heuristic and the messages that it applies to, I think it's fine.