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

Introduce Extract to Code Behind #2039

Merged
merged 2 commits into from
Jul 7, 2020
Merged

Introduce Extract to Code Behind #2039

merged 2 commits into from
Jul 7, 2020

Conversation

noahbkim
Copy link
Contributor

In order to support code action efficiently, this PR's main purpose is to integrate an additional dispatcher into the RZLS framework. Additionally, it contains a 'plugin' of sorts to this framework that handles extracting @code blocks from .razor files into backing C#.

I merged with master preemptively to double check compatibility, so please ignore diffs from that.

This PR registers two endpoints with the RZLS. The first is the CodeActionEndpoint, which is a part of the O# specifications. This endpoint asks all of its registered providers (right now there is only one for ExtractToCodeBehind, but in general they are dynamically registered as RazorCodeActionProvider services) if they have any code actions to provide. All providers that apply return a command for razor/runCodeAction with params for a custom endpoint.

If the user chooses any of the code actions provided this way, the razor/runCodeAction is executed by VSCode and a new query is sent to the language server to "resolve" or compute the full set of changes invoked by the aforementioned params. This is handled by the CodeActionResolutionEndpoint, which passes the data in the params to a correspondingly registered RazorCodeActionResolver based on a shared Action constant. This returns a serialized WorkspaceEdit, which is applied on the client side.

Addresses dotnet/aspnetcore#22239 (in this specific format)

@noahbkim noahbkim added tooling-only * NO MERGE * Do not merge this PR as long as this label is present labels Jun 16, 2020
@dnfadmin
Copy link

dnfadmin commented Jun 16, 2020

CLA assistant check
All CLA requirements met.

@noahbkim noahbkim marked this pull request as draft June 16, 2020 16:56
@NTaylorMullen
Copy link
Contributor

Wow i'm surprised that for Draft PRs it runs checks. Interesting!
image

@noahbkim
Copy link
Contributor Author

I switched it to a draft after publishing because I didn't see the option first 😅

Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Overall design looks great! Left a few high level comments. Let us know once you're ready for a more in-depth review 😄

@ajaybhargavb
Copy link
Contributor

Wow i'm surprised that for Draft PRs it runs checks. Interesting!

It has always been that way. It's super useful for investigating flaky tests on the CI.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Mostly just nit-picks and stuff that probably gets cleaned up before you leave draft anyway, nothing foundational.

@noahbkim noahbkim force-pushed the noahbkim/refactoring branch from 753524c to 9fd4ee9 Compare June 22, 2020 19:38
@noahbkim noahbkim removed the * NO MERGE * Do not merge this PR as long as this label is present label Jun 22, 2020
@noahbkim noahbkim marked this pull request as ready for review June 25, 2020 17:24
@noahbkim noahbkim force-pushed the noahbkim/refactoring branch from afeb79e to 2809fec Compare June 25, 2020 17:33
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Pretty much all of my comments are code-style nitpicks. The bones of the code look solid.

Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

Looks really good. I had a few question and mostly minor style comments.

private readonly FilePathNormalizer _filePathNormalizer;
private readonly ILogger _logger;

private static readonly Range StartOfDocumentRange = new Range(new Position(0, 0), new Position(0, 0));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static readonly Range StartOfDocumentRange = new Range(new Position(0, 0), new Position(0, 0));
private static readonly Range StartOfDocumentRange = default;

?

Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

Looks great! 👏

Just some minor comments / changes / formatting.

Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Only minor comments. Looks super good Noah!

@ryanbrandenburg
Copy link
Contributor

This is still approved by me, but let me know before you merge this. I'm doing a VS insertion and I'm told this PR can't go in with that.

Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

Looks great 🎉 🍾 . Just minor nits and one question for my own understanding.

@noahbkim noahbkim force-pushed the noahbkim/refactoring branch from a87dfc6 to c6a4401 Compare July 6, 2020 17:40
@noahbkim noahbkim force-pushed the noahbkim/refactoring branch from c6a4401 to 368d418 Compare July 6, 2020 18:08
@noahbkim noahbkim closed this Jul 6, 2020
@noahbkim noahbkim reopened this Jul 6, 2020
@noahbkim noahbkim merged commit 9870e34 into master Jul 7, 2020
@noahbkim noahbkim deleted the noahbkim/refactoring branch July 7, 2020 16:54
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.

7 participants