Filter out code action errors #1904
Conversation
Address #1889
|
@PEZ I just said "addressing" the issue since the root cause still remains to be fixed upstream. See the changelog entry too. I can change that if you want. |
|
|
||
| async function calvaShowErrorMessage(message: string) { | ||
| const exlusionRegexps = [/Request textDocument\/codeAction failed./]; | ||
| if (!exlusionRegexps.some((re) => re.test(message))) { |
There was a problem hiding this comment.
We should probably only run exclusions when document.languageId !== 'clojure'.
There was a problem hiding this comment.
Anything coming through with that message is coming from clojure-lsp / Calva. I don't think this can have any effect on other extensions or other lsp servers, if that's what you're concerned about.
There was a problem hiding this comment.
With the above in mind, I don't think that check is necessary.
There was a problem hiding this comment.
That is not a safe assumption. CodeActions are part of LSP spec, and not limited to Clojure. While the odds of some other LSP is using the exact same error message is low, it is definitely not zero.
There was a problem hiding this comment.
Also the way this is written implies readiness to filter additional errors in the future. Without verifying Clojure, we need to be very careful and precise with future regex patterns.
There was a problem hiding this comment.
Awesome. Thanks for checking. And bigger thanks for finding and implementing this.
There was a problem hiding this comment.
I'm curious about the way to verify. Wouldn't it have worked to just use a the command Joyride: Run Clojure Code to pop up a matching error message? Sounds like you did something more involved, @bpringe? Super happy you checked it, just curious about if I missed something, or maybe if there is info we could add to the Joyride docs.
There was a problem hiding this comment.
To be honest, I haven't used joyride yet, and instead of learning the ropes I just did what I knew 😄. But maybe testing code that was compiled into the extension was a better test to be sure there's not some difference in how that command runs code? I don't know.
There was a problem hiding this comment.
That's good to know. I think we have some work to do with Joyride to relay just how easy it is to start using it. 😄
If the test was to use vscode/window.showErrorMessage then this would have been tested just as well using Joyride. Everything that's done in a Joyride script is done in the context of the Joyride extension. FYI.
There was a problem hiding this comment.
Good to know! Thanks. Also, having not tried to get into Joyride yet, I don't know how easy it might be to get started, so I'm by no means suggesting it's not easy.
| } | ||
| } | ||
|
|
||
| export function activate() { |
There was a problem hiding this comment.
This should return a disposable, that can be pushed into context.subcriptions, to restore built in showErrorMessage.
There was a problem hiding this comment.
Are you suggesting we create a class that implements Disposable, and then create a dispose method that restores the built in showErrorMessage?
Considering I don't think this affects any other extensions, I don't see the need for that, but if we did want to restore the built-in showErrorMessage, I'd rather add a deactivate function. So in essence, we're activating and deactivating overrides.
There was a problem hiding this comment.
This not about potentially making this change optional for users. It about being good citizens, when for example, someone uninstalls the extension.
All of the numerous calls to context.subcriptions.push in extension.ts are to provide disposables to VS Code. When/if it needs to, Code can call dispose method of each disposable in that collection, to break down all of the Calva's infrastructure, and release things for garbage collection or undo global changes (like hijacking the error printer).
It's just one more line in this function...
return {dispose: () => vscode.window.showErrorMessage = vscodeShowErrorMessage}... and then in extension.ts wrap in a push to subscriptions.
context.subscriptions.push(overrides.activate())
There was a problem hiding this comment.
This not about potentially making this change optional for users.
I wasn't suggesting optionality for users, just to be clear.
It about being good citizens, when for example, someone uninstalls the extension.
If I thought this would affect other extensions, I wouldn't make this change, at least not as it is, and maybe not at all.
All of the numerous calls to context.subcriptions.push in extension.ts are to provide disposables to VS Code.
I'm aware of this. I've been a maintainer of Calva for quite some time now 😃. I appreciate your reviewing and concerns, though.
Also, thanks for reminding me that we don't need a class and can just use an object literal with a dispose property.
What has changed?
({}), if you add:in the middle, the error pops up. With this PR, the error is filtered out and no popup is shown.Addresses #1889.
My Calva PR Checklist
devbranch. (Or have specific reasons to target some other branch.)published. (Sorry for the nagging.)[Unreleased]entry inCHANGELOG.md, linking the issue(s) that the PR is addressing.[ ] Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.[ ] Added to or updated docs in this branch, if appropriate[ ] Figured if the change might have some side effects and tested those as well.[ ] Smoke tested the extension as such.[ ] Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on theci/circleci: buildtest.[ ] If I am fixing the issue, I have used GitHub's fixes/closes syntax[ ] Created the issue I am fixing/addressing, if it was not present.npm run prettier-format)npm run eslintbefore creating your PR, or runnpm run eslint-watchto eslint as you go).Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik