Merge features/extract-to-component to main #11019

ryzngard · 2024-10-15T00:33:14Z

New PR because apparently the branch is restricted so I can't handle merge conflicts...

Adding work from @marcarro into main 🥳

…actToComponent

…o general GenerateUniquePath helper

…nCommand'

Merge main into feature/extract-to-component

dotnet#10578) ### Summary of the changes Part of the implementation of the _Extract To Component_ code action. Functional in one of the two cases, when the user is not selecting a certain range of a Razor component, but rather when the cursor is on either the opening or closing tag.

### Summary of the changes Keep feature branch up to date - Fixes:

…10651) ### Summary of the changes Addition to feature wherein razor component dependencies are now identified, and their corresponding `@usings` are now also extracted into the new document. ## **_Important!_** Notes: Some changes in this PR are further rectified in dotnet#10760, such as: `AddComponentDependenciesInRange` does not take actionParams anymore. In general, most (if not all) of the internal methods that take `actionParams` as a parameter were moved to the resolver, where each method returns a collection of appropiate objects.

Clean up some of the extract to component code. Move more work to the resolver, use pooled objects where applicable, and try to make the provider cleaner when creating the ExtractToComponentCodeActionParams

davidwengier

The only reason I'm not approving is the confirmation around the behaviour when there is C# code inside a selection. I personally think that is really important, but perhaps there was a decision I'm not aware of/remembing.

davidwengier · 2024-10-15T03:02:37Z

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/FileUtilities.cs

+        // Add check for rooted path in the future, currently having issues in platforms other than Windows.
+        // See: https://github.com/dotnet/razor/issues/10684


Does this comment apply? IsPathRooted isn't used here.

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

davidwengier · 2024-10-15T03:07:27Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+        }
+
+        var startElementNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
+        if (startElementNode is not { EndTag: not null } || LocationOutsideNode(context.StartLocation, startElementNode))


Would be good to comment what this is (not) looking for. The double negative might be confusing me, but does this mean we don't extract self-closing tags?

hmm... that's a good point. I don't believe that check is needed. Maybe if endElement == startElement then we don't want to extract a single self-closing tag... but it should be a valid start location. I'll remove the check and add a test

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

davidwengier · 2024-10-15T03:32:24Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

+        };
+    }
+
+    private static IEnumerable<string> GetUsingsInDocument(RazorSyntaxTree syntaxTree)


Is there an issue to have this just include the minimum set?

no, I'll create one. There was code to do that but I am not sure it was right and there were no tests so I removed for now.

#11025

...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/CodeActionEndToEndTest.NetFx.cs

...Core.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToComponentCodeActionProviderTest.cs

…tions/Razor/ExtractToComponentCodeActionResolver.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

…o merge_to_main

ryzngard · 2024-10-17T01:35:37Z

This is worth a fresh pass @davidwengier . I changed a few things that came up as I added tests :)

davidwengier

Love the extra tests!

Feels like this one will be akin to formatting, in that we'll find weird edge cases and keep building our test collection out over time.

davidwengier · 2024-10-17T02:43:59Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+        => node.Kind is
+                SyntaxKind.MarkupElement or
+                SyntaxKind.MarkupTagHelperElement or
+                SyntaxKind.CSharpCodeBlock;


I think this is probably okay for now, but the Razor syntax tree is annoying and CSharpCodeBlock is pretty ubiquitous. I wonder if it's worth being more explicit with the check here, or at least logging an issue for follow up. I expect this will be a source of future quirks. eg given @attribute [StreamR$$endering], the first CSharpCodeBlock ancestor of the cursor is " [StreamRendering]" and in <div>@curren$$tCount</div> it's @currentCount, though one could argue that is extract-able.

I don't feel super strongly about it, though, because it's very much in the "if it hurts when you do that, don't do that" category IMO. Assuming the editor implements undo correctly anyway :)

Yea... I wanted to keep it open until we find cases where it's hurting too much. Then add more tests and improve :)

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

davidwengier · 2024-10-17T02:56:58Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

+        // Keep passing characters until either we reach the root indendation level
+        // or we would consume a character that isn't whitespace. This does make assumptions
+        // about consistency of tabs or spaces but at least will only fail to unindent correctly


Consider extracting all of the de-dent logic (ie, lines 96 - 110) to its own method (maybe even in FormattingUtilities?) and putting this comment, or at least the 2nd sentence, at the start of it. It really sets the expectations for what otherwise appears to be an surprising algorithmic choice.

davidwengier · 2024-10-17T03:00:59Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

        }

-        var extractedText = text.GetSubTextString(new TextSpan(actionParams.Start, actionParams.End - actionParams.Start)).Trim();
-        builder.Append(extractedText);
+        var indentation = GetIndentation(actionParams.Start, text);


I don't think I understand why this starts at the extract start point. Shouldn't the base indentation be calculated by starting at the 0th character of the line, and count forwards until it hits non-whitespace.

<div> <span> Hello </span> [|<span> World </span>|] </div>

Perhaps a pathological example, but pretty sure this would calculate the base indentation as 1, but logically it should be 4.

Also, if the extraction is all on one line, then should probably skip de-denting entirely. eg:

<div> [|<span> Hello </span>|] </div>

Yea, I might be a bit too naive. The dedent logic should probably be something more like

If all content on same line, don't dedent

Find the line with the left most non-whitespace character. Calculate that indentation

Dedent everything by that indentation

The pathological case will still be wrong but maybe closer to right? Could also take advantage of the syntax tree and make sure paired start/ends match but that gets closer into making a static formatter for razor than I currently have the brain space for

The other approach could be to get the actual indent string from the first line, and remove it from the start of subsequent lines, if it exists, and simply ignore lines that have a space/tab mix. Of the three, Not sure either is necessarily "right"

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/CodeActionEndToEndTest.NetFx.cs

...Core.Razor.LanguageServer.Test/CodeActions/Razor/ExtractToComponentCodeActionProviderTest.cs

marcarro and others added 30 commits July 3, 2024 13:28

Added basic extract to component functionality on cursor over html tag

9ba71db

PR Feedback

7e3ef75

Merge branch 'features/extract-to-component' into dev/t-hearroyo/Extr…

74956e1

…actToComponent

Changed JToken Use in PR

bce001c

PR Feedback and added GenerateUniquePath

60d5881

Changed usage from GenerateCodeBehindPath and GenerateComponentPath t…

8ab2133

…o general GenerateUniquePath helper

More PR Feedback

c63e51c

Reverted nameof change for LanguageServerConstant 'EditBasedCodeActio…

09435b4

…nCommand'

Update feature branch (dotnet#10652)

921aef1

Merge main into feature/extract-to-component

Changed class constructor to primary constructor

afc61ea

Merge from main

6d8e44c

PR Feedback and update according to new IRazorCodeActionProvider API

2908e6c

Correct for test issues

9d7a565

Fixed test issue

371b09a

Update feature branch (dotnet#10680)

6f56d75

### Summary of the changes Keep feature branch up to date - Fixes:

Arranging selection logic

142c3ed

Basic select range feature: practically not functional

ef995cd

Completed and corrected selected range extraction functionality

a81a177

Tried fixing nullability issues

ea0ac53

Commented out incomplete test. Must be finished

0deb915

Rebased to feature branch and updated according to new API

46932b3

Changed null assert to empty assert according to ci test results

a939694

Refactored element selection logic into methods for easier readability

a814ca8

Removed unnecessary using

8ba59f2

Added check for IsInsideProperHtmlContent

cd6b24a

Removed unnecessary iteration variable in FindContainingSiblignPair

55669a7

nits and test fixes

8ee84de

Functioning tests and fixed selection issue

eba1b01

Added null check

6656f7a

marcarro and others added 8 commits August 26, 2024 19:47

Nits. I'm not sure why tests are failing

851fb45

Merge branch 'main' into features/extract-to-component

be1f0d7

Fix merge

ea556b9

Fix moq in tests

b776c60

Merge main into features/extract-to-component (dotnet#10948)

17dbcdb

Some Cleanup For ExtractToComponent (dotnet#11008)

e060ce0

Clean up some of the extract to component code. Move more work to the resolver, use pooled objects where applicable, and try to make the provider cleaner when creating the ExtractToComponentCodeActionParams

Merge branch 'main' into features/extract-to-component

fccbbfc

ryzngard requested review from a team as code owners October 15, 2024 00:33

davidwengier reviewed Oct 15, 2024

View reviewed changes

ryzngard and others added 6 commits October 15, 2024 12:17

Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeAc…

da1b19b

…tions/Razor/ExtractToComponentCodeActionResolver.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeAc…

95123dc

…tions/Razor/ExtractToComponentCodeActionResolver.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

PR feedback

f07da46

Merge branch 'merge_to_main' of https://github.com/ryzngard/razor int…

fa3b2bf

…o merge_to_main

Add expected original document to E2E tests

c873dc6

More tests. Fix some bugs. Dedent new file

94407a0

davidwengier approved these changes Oct 17, 2024

View reviewed changes

ryzngard added 2 commits October 17, 2024 19:23

PR feedback and move unindent logic to FormattingUtilities

d64e2b3

USINGS....

a49c67d

ryzngard merged commit 5269c1c into dotnet:main Oct 18, 2024
12 checks passed

ryzngard deleted the merge_to_main branch October 18, 2024 22:41

dotnet-policy-service bot added this to the Next milestone Oct 18, 2024

dotnet-bot mentioned this pull request Oct 22, 2024

[Automated] PRs inserted in VS build main-35421.179 #11056

Closed

davidwengier mentioned this pull request Oct 25, 2024

Update razor package dotnet/vscode-csharp#7692

Merged

This was referenced Oct 25, 2024

[Automated] PRs inserted in VS build feature.debugger.main-35424.209 #11093

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35428.160 #11107

Closed

phil-allen-msft modified the milestones: Next, 17.13 P1 Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge features/extract-to-component to main #11019

Merge features/extract-to-component to main #11019

ryzngard commented Oct 15, 2024 •

edited

Loading

davidwengier left a comment

davidwengier Oct 15, 2024

davidwengier Oct 15, 2024

ryzngard Oct 15, 2024

davidwengier Oct 15, 2024

ryzngard Oct 15, 2024

ryzngard commented Oct 17, 2024

davidwengier left a comment

davidwengier Oct 17, 2024

ryzngard Oct 18, 2024

davidwengier Oct 17, 2024

davidwengier Oct 17, 2024

ryzngard Oct 18, 2024

davidwengier Oct 18, 2024

		// Add check for rooted path in the future, currently having issues in platforms other than Windows.
		// See: https://github.com/dotnet/razor/issues/10684

Merge features/extract-to-component to main #11019

Merge features/extract-to-component to main #11019

Conversation

ryzngard commented Oct 15, 2024 • edited Loading

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Oct 17, 2024

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Oct 15, 2024 •

edited

Loading