Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames UiError and UiErrorDetails to FrameworkError and FrameworkErrorDetails across the ui_primitives package, updating foundation, diagnostics, and value notifier components. It also includes a version bump and a new test suite for error reporting. Feedback suggests restoring removed documentation links, updating remaining string literals for consistency, and including full URLs in TODO comments.
| /// Error class used to report Framework-specific assertion failures and | ||
| /// contract violations. |
There was a problem hiding this comment.
The See also documentation block linking to Flutter's error handling documentation was removed. Since this class is analogous to Flutter's FlutterError, it would be beneficial to retain this link for developers familiar with the Flutter framework.
/// Error class used to report Framework-specific assertion failures and
/// contract violations.
///
/// See also:
///
/// * <https://docs.flutter.dev/testing/errors>, more information about error
/// handling in Flutter.| ErrorSummary('Empty FlutterError'), | ||
| ]), | ||
| ) { | ||
| assert( | ||
| diagnostics.first.level == DiagnosticLevel.summary, | ||
| UiError.fromParts(<DiagnosticsNode>[ | ||
| FrameworkError.fromParts(<DiagnosticsNode>[ | ||
| ErrorSummary('FlutterError is missing a summary.'), | ||
| ErrorDescription( | ||
| 'All FlutterError objects should start with a short (one line) ' | ||
| 'summary description of the problem that was detected.', |
There was a problem hiding this comment.
The error messages in the assertions still refer to FlutterError. They should be updated to FrameworkError to be consistent with the refactoring.
ErrorSummary('Empty FrameworkError'),
]),
) {
assert(
diagnostics.first.level == DiagnosticLevel.summary,
FrameworkError.fromParts(<DiagnosticsNode>[
ErrorSummary('FrameworkError is missing a summary.'),
ErrorDescription(
'All FrameworkError objects should start with a short (one line) '
'summary description of the problem that was detected.',| r'#0 getSampleStack\.<anonymous closure> \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'#2 getSampleStack \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'#3 main \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'(.+\n)+', // TODO(ianh): when fixing #4021, also filter out frames from the test infrastructure below the first call to our main() |
There was a problem hiding this comment.
This TODO refers to a Flutter issue by its number. It would be more helpful to include the full URL to the issue for better context and traceability, like so: // TODO(ianh): when fixing https://github.com/flutter/flutter/issues/4021, also filter out frames...
| r'(.+\n)+', // TODO(ianh): when fixing #4021, also filter out frames from the test infrastructure below the first call to our main() | |
| r'(.+\n)+', // TODO(ianh): when fixing https://github.com/flutter/flutter/issues/4021, also filter out frames from the test infrastructure below the first call to our main() |
| console.join('\n'), | ||
| matches( | ||
| _setPath( | ||
| r'^══╡ EXCEPTION CAUGHT BY FLUTTER FRAMEWORK ╞═════════════════════════════════════════════════════════\n' |
There was a problem hiding this comment.
The test expects 'FLUTTER FRAMEWORK' in the error output. This comes from the default library name in FrameworkErrorDetails. Given the refactoring to FrameworkError, it would be more consistent to use a more generic name like 'FRAMEWORK' here. To achieve this, you can pass a custom library name to FrameworkErrorDetails in the test setup (e.g., on line 111) and update this expectation. This applies to other similar expectations in this test file.
| r'^══╡ EXCEPTION CAUGHT BY FLUTTER FRAMEWORK ╞═════════════════════════════════════════════════════════\n' | |
| r'^══╡ EXCEPTION CAUGHT BY FRAMEWORK ╞═════════════════════════════════════════════════════════\n' |
| r'#0 getSampleStack\.<anonymous closure> \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'#2 getSampleStack \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'#3 main \([^)]+$thisTestPath:[0-9]+:[0-9]+\)\n' | ||
| r'(.+\n)+', // TODO(ianh): when fixing #4021, also filter out frames from the test infrastructure below the first call to our main() |
There was a problem hiding this comment.
This TODO refers to a Flutter issue by its number. It would be more helpful to include the full URL to the issue for better context and traceability, like so: // TODO(ianh): when fixing https://github.com/flutter/flutter/issues/4021, also filter out frames...
| r'(.+\n)+', // TODO(ianh): when fixing #4021, also filter out frames from the test infrastructure below the first call to our main() | |
| r'(.+\n)+', // TODO(ianh): when fixing https://github.com/flutter/flutter/issues/4021, also filter out frames from the test infrastructure below the first call to our main() |
1 participant
No description provided.