-
Notifications
You must be signed in to change notification settings - Fork 47
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 codeAction for bs imports #347
Conversation
/** | ||
* Given a Diagnostic or BsDiagnostic, return a copy of the diagnostic | ||
*/ | ||
public toDiagnostic(diagnostic: Diagnostic | BsDiagnostic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the BsDiagnostic
objects contain a file
reference which has a program
, which has an array of files (including the one we just referenced). This causes a recursive error during JSON.stringify when the language server sends the code actions or diagnostics to the client. This function will clone a diagnostic, but only keep the officially supported properties. Thus preventing the JSON.stringify crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any time we send diagnostics to the client, we should run them through this function.
* A map of function statements, indexed by fully-namespaced lower function name. | ||
*/ | ||
public get functionStatementLookup() { | ||
if (!this._functionStatementLookup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compute this on-demand, so we don't pay the price until we need it. (currently only used during code actions)
const { component } = this.file.parser.ast; | ||
//inject new attribute after the final attribute, or after the `<component` if there are no attributes | ||
const pos = (component.attributes[component.attributes.length - 1] ?? component.tag).range.end; | ||
this.codeActions.push( | ||
codeActionUtil.createCodeAction({ | ||
title: `Extend "Group"`, | ||
// diagnostics: [diagnostic], | ||
diagnostics: [diagnostic], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can send diagnostics for these actions now that we are sanitizing them before sending them in the language server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the diagnostic give?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually know. I looked through the docs and they don't explain it at all.
@elsassph I've finished refactoring the codeActions plugin functionality. Now there's only a single This is technically a breaking change for the codeActions plugin api, but it was only recently introduced, and the entire plugin system is still in beta so we are still at liberty to break it as needed. This is the first step towards object-based plugin events (the rest will land in #330). It's ready for another review. |
@elsassph i addressed one of your concerns, but I didn't address #347 (comment) because I wanted to verify that you thought it was actually useful to duplicate the typecast there (I don't see much value) |
Support for brighterscript code action to suggest file imports for missing functions.