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

Support code folding for argument lists #71064

Merged
merged 13 commits into from
Feb 17, 2024

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 2, 2023

Fixes #67929

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 2, 2023 22:25
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 2, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 2, 2023
@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Could you review this small PR? (assuming the linked issue is something you want to fix)

using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Structure;

namespace Microsoft.CodeAnalysis.CSharp.Structure
Copy link
Member

Choose a reason for hiding this comment

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

file scoped namespace.

""";

await VerifyBlockSpansAsync(code);
}
Copy link
Member

Choose a reason for hiding this comment

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

test with just two args on different lines.

var code = """
var x = M$${|span:(
"",
"")|};
Copy link
Member

Choose a reason for hiding this comment

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

what's the experience you have in the IDE if you have:

var v = M(N(
   "",
   ""));

In other words, how does nesting impact things, when the outer/inner are on the same start/end lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi I wasn't able to get RoslynDeployment work properly. https://www.youtube.com/watch?v=ND8dDO6oJ4U see around 38:00

@CyrusNajmabadi
Copy link
Member

I'll try this out locally.

@CyrusNajmabadi
Copy link
Member

So the experience is unfortunately not ok currently:

collapse

An option it to make it so that the innermost or outermost of a set of invocations with the same start line is collapsable, instead of all of htem.

Probably the innermost makes the most sense so you still see as much code as possible.

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Is it better now?

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 15, 2023 20:19
return;
}

switch (_count)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 18, 2023

Choose a reason for hiding this comment

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

Basically:

            Span<T> span = MemoryMarshal.CreateSpan(ref _item0, this.Count);
            span.Sort(comparison);

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not available in netstandard2.0 😕

Copy link
Member

Choose a reason for hiding this comment

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

Gah. @stephentoub Any recommendations on how to best sort a span in netstandard2.0?

@@ -205,6 +206,69 @@ public void TestReverseContents([CombinatorialRange(0, 6)] int initialItems)
Assert.Equal(array[i], initialItems - 1 - i);
}

[Fact]
public void TestSort()
Copy link
Member

Choose a reason for hiding this comment

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

i would take everything out related to sorting TempArray. We don't even relaly need temparray for block-structure. That's an array that's good when you commonly have <4 elements. But for block structure, we'd almost alwayshave more. So we can just pass an ArrayBuilder along instead).

Copy link
Member

Choose a reason for hiding this comment

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

or, we can keep this. i'm on the fence.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi I had the same feeling the TemporaryArray might not be very suitable for this scenario, but I preferred to limit the scope of my changes.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

Changes under Compilers LGTM (commit 9)

@@ -54,7 +54,7 @@ using System.Linq;|}", "imports", "...")>
</Workspace>, openDocuments:=False, composition:=TestLsifOutput.TestComposition)

Dim annotatedLocations = Await AbstractLanguageServerProtocolTests.GetAnnotatedLocationsAsync(workspace, workspace.CurrentSolution)
Dim expectedRanges = annotatedLocations("foldingRange").Select(Function(location) CreateFoldingRange(rangeKind, location.Range, collapsedText)).OrderBy(Function(range) range.StartLine).ToArray()
Dim expectedRanges = annotatedLocations("foldingRange").Select(Function(location) CreateFoldingRange(rangeKind, location.Range, collapsedText)).OrderByDescending(Function(range) range.StartLine).ToArray()
Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the returned spans changed with the changes in this PR. I think the order shouldn't matter. So, I updated this test (and some others) to assert against the new order.

I think the tests could even ignore the order when comparing, but I didn't do that for now.

@CyrusNajmabadi Let me know your thoughts.

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Can you take a look please? Thanks!

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Friendly reminder on this PR. Thanks!

@CyrusNajmabadi
Copy link
Member

Yup yup. Thanks for the reminder!

@CyrusNajmabadi CyrusNajmabadi merged commit cab5afb into dotnet:main Feb 17, 2024
30 checks passed
@Youssef1313 Youssef1313 deleted the code-fold-argumentlist branch February 17, 2024 21:50
@ghost ghost added this to the Next milestone Feb 17, 2024
@jjonescz jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request]: Code folding for invocation expressions that span multiple lines
4 participants