Skip to content
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 support #298

Merged
merged 27 commits into from
Feb 18, 2021
Merged

Add codeAction support #298

merged 27 commits into from
Feb 18, 2021

Conversation

TwitchBronBron
Copy link
Member

@TwitchBronBron TwitchBronBron commented Feb 2, 2021

This adds the ability to send code actions from the language server to the editor.

I created a helper (util.createCodeAction) to simplify creating code actions (because they're kind of annoying to create.

TODO

  • support scope-based code actions

@TwitchBronBron TwitchBronBron changed the title Add file-based and plugin code action support Add codeAction support Feb 3, 2021

private addMissingExtends() {
//add the attribute at the end of the first attribute, or after the `<component` if no attributes
const pos = (this.file.parser.ast.component.attributes[0] ?? this.file.parser.ast.component.tag).range.end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the position is correct for a tag without any attribute. You probably should just always insert at the end of the tag instead of trying to find the first attribute.

Also maybe have some const { component } = this.file.parser.ast; for readability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so we don't occasionally insert two consecutive whitespaces. It's easier to reason about the whitespace after an attribute. Although I do agree that we should add this to the very end of the tag, so I adjusted this to find the FINAL attribute first, and then after the <component object second if there are no attributes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an attribute or a tag should be done with a helper function BTW, so it's testable and reusable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll do that.

@TwitchBronBron
Copy link
Member Author

@elsassph I really want to get this feature merged, but I do agree that we should not leak internal xml ast data. So, I have ripped out that specific codeAction. So please give this PR a final review, and approve if you have no further concerns. I'll continue working on those other comments in separate PRs.

@TwitchBronBron TwitchBronBron marked this pull request as ready for review February 15, 2021 20:58
Copy link
Contributor

@elsassph elsassph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little concern about accumulating code action requests

@@ -543,6 +550,22 @@ export class LanguageServer {
return item;
}

private async onCodeAction(params: CodeActionParams) {
//ensure programs are initialized
await this.waitAllProgramFirstRuns();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These awaits will potentially accumulate code action requests - should there instead be a mechanism to ignore codeactions when the file isn't ready? We may "miss" some actions but just moving the cursor around will produce then again.

Copy link
Member Author

@TwitchBronBron TwitchBronBron Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language server protocol might already account for this and prevent additional requests if the first one hasn't resolved yet. So I'll test that first, and if we DO get multiple requests at the same time, then I'll add some code to ignore code actions that arrive before the programs are finished with their first runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested, and I can't get these to queue up. There's a waitAllProgramFirstRuns() in the onInitialized, so the vscode doesn't even send codeAction events until that's resolved, so this line is really a "just in case" measure.

@TwitchBronBron TwitchBronBron merged commit 674557b into master Feb 18, 2021
@TwitchBronBron TwitchBronBron deleted the getCodeActions branch February 18, 2021 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants