This is an archive of the discontinued LLVM Phabricator instance.

[clangd][vscode] Surface the error when applying tweaks fails
ClosedPublic

Authored by hokein on Aug 14 2019, 4:17 AM.

Details

Summary

The current behavior for a failed request is just to log it in the
output panel. When applyTweak fails for whatever reason, users usually don't get
informed (unless they open the output panel and dig the log).

this patch is to surface these errors by prompting up a message diag.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Aug 14 2019, 4:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 4:17 AM

Surfacing errors to the users in those cases is definitely something we need to do, thanks!
How do they look in practice? In particular, should we add more context information (either in clangd or in the VSCode extension) to those errors, now that we know they are shown to the users?
Something like Failed to apply a fix: input range is invalid vs input range is invalid.

How do they look in practice? In particular, should we add more context information (either in clangd or in the VSCode extension) to those errors, now that we know they are shown to the users?
Something like Failed to apply a fix: input range is invalid vs input range is invalid.

I don't have a very strong preference on adding more context, the current messages seem good enough to me, something like:

  • "edits were not applied: <reason>"
  • "Could not expand type of lambda expression:"
  • "Could not obtain range of the 'else' branch. Macros?"
ilya-biryukov accepted this revision.Aug 14 2019, 5:56 AM

Thanks for providing examples of error messages, also agree they look fine.
If we find bad error messages later, we could revisit the presentation strategy.
LGTM

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
109 ↗(On Diff #215079)

Is this how default implementation behaves? Is there a way to call into it instead of re-implementing?
If so, it would be nice to do so. If not, I'm also ok if we keep it.

This revision is now accepted and ready to land.Aug 14 2019, 5:56 AM
hokein updated this revision to Diff 215107.Aug 14 2019, 6:33 AM
hokein marked an inline comment as done.

avoid re-implementing of the method.

hokein added inline comments.Aug 14 2019, 6:34 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
109 ↗(On Diff #215079)

manage to figure out a way to do it. We have to define a subclass of LanguageClient, and override logFailedRequest.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 6:37 AM