Create custom DiagnosticsNode type for DevTools deep links. #74455
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// [value] will return the String representation of the DevTools url. | ||
| /// | ||
| /// The [description] and [value] arguments must not be null. | ||
| DevToolsDeepLinkDiagnosticsNode(String description, String value) : |
There was a problem hiding this comment.
Consider renaming parameter value to url and making it named.
| super(name, null, description: message, style: style, level: level); | ||
| } | ||
|
|
||
| /// Debugging message for DevTools deep links. |
There was a problem hiding this comment.
Top level comments should ideally say something more than just the words from the class name in another order. Clarify why it makes sense to indicate that a link is to Flutter DevTools vs any other deep link you might have in the app.
Change the class name to be consistent with the other class names.
For example
DevToolsDeepLinkProperty would be better.
| super(name, null, description: message, style: style, level: level); | ||
| } | ||
|
|
||
| /// Debugging message for DevTools deep links. |
There was a problem hiding this comment.
Move this class from diagnostics.dart to widget_inspector.dart. This isn't general purpose enough to be in foundation
| WidgetInspectorService.instance._devToolsInspectorUriForElement(target); | ||
| if (devToolsInspectorUri != null) { | ||
| devToolsDiagnostic = DiagnosticsNode.message( | ||
| devToolsDiagnostic = DevToolsDeepLinkDiagnosticsNode( |
There was a problem hiding this comment.
consider moving some of the logic from _devToolsInspectorUriForElement into DevToolsDeepLink as it is tightly coupled and that would make the DevToolsDeepLink class a little less surprising than if its public constructor takes an arbitrary URL which might or might not be from DevTools.
| class DevToolsDeepLinkDiagnosticsNode extends StringProperty { | ||
| /// Create a diagnostics property that displays a deep link to DevTools. | ||
| /// | ||
| /// [value] will return the String representation of the DevTools url. |
There was a problem hiding this comment.
nit: doc sentences should be grammatically correct and start with a capital letter, e.g. "The [value] will..."
|
Addressed review comments and refactored some things. Represented the diagnostic value as a map so I can add useful parameters for IDEs to consume (other wise they would have had to parse information from the url). PTAL. |
| /// diagnostic pertains to. | ||
| /// `objectId` is an optional id specifying the data object in Flutter DevTools | ||
| /// that this diagnostic pertains to. | ||
| class DevToolsDeepLinkProperty extends DiagnosticsProperty<Map<String, String>> { |
There was a problem hiding this comment.
This should not be a property on Map<String, String>
instead make
DevToolsDeepLink extend Diagnosticable.
Then it is clean enough to add the extra json you want and specify the tostring you needed.
| 'missingIfNull': false, | ||
| 'propertyType': 'String', | ||
| 'defaultLevel': 'info', | ||
| 'value': 'the-deep-link', |
IDEs will look for this specific diagnostic node type in order to prompt the user to inspect the errors in DevTools.
cc @helin24 @DanTup