[Property Editor] Handle errors from the Analysis Server #8818
Conversation
elliette
requested review from
bkonyi and
kenzieschmoll
and removed request for
a team and
bkonyi
| /// These should be kept in sync with those listed in: | ||
| /// pkg/analysis_server/lib/src/lsp/constants.dart |
There was a problem hiding this comment.
can we have a test that covers this?
There was a problem hiding this comment.
Hmm I was trying to think how we could test this. We can't pull in analysis_server as a dependency (otherwise I would just use the error definitions there instead) so I'm not sure how we could have a test to check they are in sync. Any ideas?
There was a problem hiding this comment.
can we load the raw content of this file from GitHub and parse it? A bit hacky but still could be worth it to have a way to catch changes
There was a problem hiding this comment.
Filed #8824 and added TODOs
| final propertyName = property.name; | ||
| switch (errorType) { | ||
| case EditArgumentError.editArgumentInvalidParameter: | ||
| return 'Invalid parameter: $propertyName.'; |
There was a problem hiding this comment.
what do you think about including the description in the EditArgumentError definitions so that the messages are defined where the enum values are? Something like:
enum EditArgumentError {
editArgumentInvalidParameter(-32017, 'Invalid parameter')
...
final String description;
General question. Seems like we use Argument in some places and Property in others. Should we pick a single term for consistency?
There was a problem hiding this comment.
what do you think about including the description in the EditArgumentError definitions so that the messages are defined where the enum values are?
Yes, that's better! Made the change.
General question. Seems like we use Argument in some places and Property in others. Should we pick a single term for consistency?
I've been trying to use argument for the layer that calls the analysis server APIs (since the API methods are called editArgument and getEditableArguments) and property for the UI layer. The "conversion" from argument to property happens here:
But I don't have a strong preference for renaming everywhere vs keeping as is!
packages/devtools_app/pubspec.yaml
Outdated
| @@ -37,6 +37,7 @@ dependencies: | |||
| http: ^1.1.0 | |||
| image: ^4.1.3 | |||
| intl: ^0.19.0 | |||
| json_rpc_2: ^3.0.0 | |||
There was a problem hiding this comment.
do we need to bring this dependency in? Is this for the RpcException type, and is this the type that DTD throws?
There was a problem hiding this comment.
Yes, this is for the RpcException type thrown by DTD. This used to be a dev dependency.
| @@ -12,7 +12,7 @@ import 'package:test/test.dart'; | |||
|
|
|||
| // Benchmark size in kB. | |||
| const bundleSizeBenchmark = 5400; | |||
| const gzipBundleSizeBenchmark = 1550; | |||
| const gzipBundleSizeBenchmark = 1650; | |||
There was a problem hiding this comment.
Bundle size is now 1552kb, therefore bumping 1550 limit to 1650
| if (errorResponse == null || errorResponse.success) return; | ||
| setState(() { | ||
| _serverError = _errorMessage(errorResponse.errorType, property: property); | ||
| _serverError = | ||
| errorResponse.errorType?.message ?? 'Encountered unknown error.'; |
There was a problem hiding this comment.
do we still want to include the property in this error message? I think it would be helpful:
'${errorResponse.errorType?.message ?? 'Encountered unknown error.'} Property: $property.'
Fixes #8817
Work towards #1948
Handles any errors from the Analysis Server in response to an
editArgumentrequest. These errors are unexpected states; we shouldn't be triggering them. Follow-up (#8774) to send these exceptions to analytics as well.