Skip to content
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

Add more structure to errors. #34684

Merged
merged 2 commits into from Jun 20, 2019

Conversation

Projects
None yet
4 participants
@jacob314
Copy link
Contributor

commented Jun 18, 2019

Description

This CL refactors more FlutterError messages to take advantage of structured error functionality.
This is a follow up to #30983

Related Issues

Replace this paragraph with a list of issues related to this PR from our [issue database]. Indicate, which of these issues are resolved or fixed by this PR.

Tests

I added tests for the error messages in multiple packages as existing test coverage was quite spotty.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

@googlebot googlebot added the cla: yes label Jun 18, 2019

@jacob314 jacob314 requested review from goderbauer and pq Jun 18, 2019

@pq

pq approved these changes Jun 19, 2019

Copy link
Contributor

left a comment

Exciting to see this all come together! 🔥

'Tried to paint a RenderObject reentrantly.\n'
'The following RenderObject was already being painted when it was '
'painted again:\n'
' ${toStringShallow(joiner: "\n ")}\n'

This comment has been minimized.

Copy link
@pq

pq Jun 19, 2019

Contributor

Nice to see these going away! 👍

throw FlutterError.fromParts(<DiagnosticsNode>[
ErrorSummary('A GlobalKey was used multiple times inside one widget\'s child list.'),
DiagnosticsProperty<GlobalKey>('The offending GlobalKey was', key),
parent.describeElement('The parent of the widgets with that key was'),

This comment has been minimized.

Copy link
@pq

pq Jun 19, 2019

Contributor

I guess we don't need to qualify describeElement with parent here?

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

This code looks consistent with the previous error message which was describing the parent. If the parent was removed the error would be describing a describing the widget with the key not its parent.

'The parent of the widgets with that key was:\n  $parent\n'

became

parent.describeElement('The parent of the widgets with that key was'),
@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';

This comment has been minimized.

Copy link
@pq

pq Jun 19, 2019

Contributor

Consider sorting these imports?

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

Removing that import. Auto-import of unimported libraries gets a bit confused in the flutter sdk.

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

Checks for unused imports also appear to be confused likely by re-exported methods that are used. For example, both material and widget re-export some methods from foundation that are indeed used. Fixing that issue would be a bit subtle.

'example, a RenderSliver cannot be the child of a RenderBox because '
'a RenderSliver does not understand the RenderBox layout protocol.',
),
ErrorDescription(''),

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

These ErrorDescription('') look kinda odd...

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

Should there maybe be a configuration option somewhere to mark paragraphs that should be surrounded by blank lines?

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

Or a specific Error DiagnosticsNode class that just represents a blank line?

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

I agree those look bad.
I'm happy to add an DiagnosticSpacer or ErrorSpacer class that is sugar for this.
Let me know if that sounds reasonable.

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

One other option would be to add a trailing '\n' to the previous ErrorDescription but that makes the intent less obvious to someone reading the message at it would be reasonable to assume that all ErrorDescription objects should just have a trailing \n.

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

I like the syntactic sugar idea.

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 19, 2019

Author Contributor

Done. Added ErrorSpacer, used it everywhere we were creating empty DiagnosticsNode objects as spacer or adhoc adding multiple line breaks at the end of sections in error messages.
Fixed a couple cases where we had accidentally extra unintended whitespace in the process and added tests.

@jacob314

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Ready for another look. I'm not sure if the existing spacing between blocks in the Semantics error message case was intentional or accidental. Left in line breaks between entries in the list in case they were considered useful. Please comment in semantics_test.dart if they should be removed.

@goderbauer
Copy link
Member

left a comment

LGTM

@@ -175,6 +175,19 @@ class ErrorHint extends _ErrorDiagnostic {
ErrorHint._fromParts(List<Object> messageParts) : super._fromParts(messageParts, level:DiagnosticLevel.hint);
}

/// An [ErrorSpacer] creates an empty [DiagnosticsNode], that can be used tune

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

nit: there seems to be a word missing? "can be used to tune"?

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 20, 2019

Author Contributor

done.

/// An [ErrorSpacer] creates an empty [DiagnosticsNode], that can be used tune
/// the spacing between other [DiagnosticsNode] objects.
class ErrorSpacer extends DiagnosticsProperty<void> {
/// Creates a empty space to insert into a list of [DiagnosticNode] objects

This comment has been minimized.

Copy link
@goderbauer

goderbauer Jun 19, 2019

Member

a -> an empty space?

This comment has been minimized.

Copy link
@jacob314

jacob314 Jun 20, 2019

Author Contributor

done

jacob314 added some commits Apr 12, 2019

@jacob314 jacob314 force-pushed the jacob314:structured_error_step_2 branch from a2148cf to c613e28 Jun 20, 2019

@jacob314 jacob314 merged commit 38f8490 into flutter:master Jun 20, 2019

39 checks passed

WIP Ready for review
Details
add2app-macos Task Summary
Details
add2app-macos
Details
analyze Task Summary
Details
analyze
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-macos Task Summary
Details
build_tests-macos
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
deploy_gallery Task Summary
Details
deploy_gallery
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs Task Summary
Details
docs
Details
flutter-build
Details
integration_tests-linux Task Summary
Details
integration_tests-linux
Details
integration_tests-windows Task Summary
Details
integration_tests-windows
Details
release_smoke_tests Task Summary
Details
release_smoke_tests
Details
tests-linux Task Summary
Details
tests-linux
Details
tests-macos Task Summary
Details
tests-macos
Details
tests-windows Task Summary
Details
tests-windows
Details
tool_tests-linux Task Summary
Details
tool_tests-linux
Details
tool_tests-macos Task Summary
Details
tool_tests-macos
Details
tool_tests-windows Task Summary
Details
tool_tests-windows
Details
web_tests-linux Task Summary
Details
web_tests-linux
Details

@jacob314 jacob314 deleted the jacob314:structured_error_step_2 branch Jun 20, 2019

goderbauer added a commit to goderbauer/flutter that referenced this pull request Jul 3, 2019

Add more structure to errors messages. (flutter#34684)
Breaking change to extremely rarely used ParentDataWidget.debugDescribeInvalidAncestorChain api changing the return type of the method from String to DiagnosticsNode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.