Make RazorDocumentMappingService easier to understand #8775

davidwengier · 2023-05-30T04:12:17Z

Sorry for the size of the PR. Recommend reviewing change-at-a-time. Most changes were done mechanically with the rename function in VS (and I only had to log 3 bugs against Roslyn while doing it!)

Things this does:

Change from "MapToProjected" and "MapFromProjected" to instead use "MapToGenerated" and "MapToHost"
Change all other instances of "virtual" or "projected" to be "generated"
Change anything that was "original" to be "hostDocument" to be clearer
Use an interface instead of an abstract class
Rename classes to get rid of the "Default" prefix
Move some code to extension methods where it didn't need to be in the service

There are still a couple of things that use the term "virtual", but I limited this PR to the document mapping service to keep things sane. Its a super important service though, so it deserves some love.

In the perfect illustration of why "Projected" is a bad term, this method was named completely backwards to what it actually does :)

I thought it would be easier to differentiate "Host" vs "Generated" in completion, versus "From" and "To"

Interestingly this method is not used outside the service. Could be removed from the interface if we really wanted I guess.

...oft.AspNetCore.Razor.LanguageServer/Completion/Delegation/DelegatedCompletionListProvider.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AbstractRazorDelegatingEndpoint.cs

...crosoft.AspNetCore.Razor.LanguageServer/Extensions/IRazorDocumentMappingServiceExtensions.cs

DustinCampbell

Fantastic! However, it feels like there's still some work to do here to update parameter and variable names to get rid of the "projection" terminology.

davidwengier · 2023-05-30T21:10:33Z

it feels like there's still some work to do here to update parameter and variable names to get rid of the "projection" terminology.

I was focusing on the API itself, so new uses would fall into the pit of success, as I thought the PR would be big enough already, but if my audience demands it, far be it from me to deny you of even more commits!

Will update :D

DustinCampbell · 2023-05-30T21:26:44Z

I was focusing on the API itself

Parameter names are API 😄

ryzngard · 2023-05-31T19:28:33Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/IRazorDocumentMappingService.cs

+    /// generated document. If the uri passed in is not for a generated document, or the range cannot be mapped
+    /// for some other reason, the original passed in range is returned unchanged.
+    /// </summary>
+    Task<(Uri MappedDocumentUri, Range MappedRange)> MapToHostDocumentUriAndRangeAsync(Uri generatedDocumentUri, Range generatedDocumentRange, CancellationToken cancellationToken);


Suggested change

Task<(Uri MappedDocumentUri, Range MappedRange)> MapToHostDocumentUriAndRangeAsync(Uri generatedDocumentUri, Range generatedDocumentRange, CancellationToken cancellationToken);

Task<(Uri HostDocumentUri, Range MappedRange)> MapToHostDocumentUriAndRangeAsync(Uri generatedDocumentUri, Range generatedDocumentRange, CancellationToken cancellationToken);

If I'm understanding this correctly, the host document URI is returned by this (under new terminology)

Yes, thats right.

ryzngard

Admittedly did not look at every file, but the renaming and new interfaces look correct to me.

davidwengier added 14 commits May 30, 2023 09:23

Rename file and type name

a5cddda

Update references

5662b4c

Extract functionality to extension methods

4eeacb6

Change abstract class to interface

6a784ba

Simplify interface

c906a81

Rename class to remove "Default" prefix

0737e01

Update references to the old class name

0095c18

Rename GetProjectedDocumentEdits

31a4166

In the perfect illustration of why "Projected" is a bad term, this method was named completely backwards to what it actually does :)

Rename TryMapFromProjectedDocumentRange

3c8f73b

I thought it would be easier to differentiate "Host" vs "Generated" in completion, versus "From" and "To"

Rename TryMapFromProjectedDocumentPosition

e913268

Interestingly this method is not used outside the service. Could be removed from the interface if we really wanted I guess.

Rename TryMapToProjectedDocumentRange

938ab19

Rename TryMapToProjectedDocumentPosition

d39e57e

Rename TryMapToProjectedDocumentOrNextCSharpPosition

f617867

Rename all other uses of "virtual" or "projected"

36b2bbc

davidwengier requested a review from a team as a code owner May 30, 2023 04:12