Try adjusting caret position to provide better results from Roslyn #8716
Conversation
| protected virtual bool TreatAnyAttributePositionAsAttributeName { get; } = false; | ||
|
|
||
| /// <summary> | ||
| /// If <see cref="TreatAnyAttributePositionAsAttributeName "/> is <see langword="true"/>, returns whether the position was adjusted | ||
| /// </summary> | ||
| protected bool DidTreatAttributePositionAsAttributeName { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// If <see cref="TreatAnyAttributePositionAsAttributeName "/> is <see langword="true"/>, returns the range of the full attribute | ||
| /// </summary> | ||
| protected Range OriginalAttributeRange { get; private set; } |
There was a problem hiding this comment.
This class is getting a little big IMO, and doing a few things, but I couldn't see a nice way to split it up, and more importantly I couldn't come up with a nice name for a class that could sit in the middle of the hierarchy and provide this functionality. AbstractRazorDelegatingEndpointThatAdjustsCaretPositionToAttributeNamesIfPossible is just silly :)
I don't think this is too egregious that we can't merge this, but its on a slippery slope. Open to feedback.
|
|
||
| protected override string CustomMessageTarget => RazorLanguageServerCustomMessageTargets.RazorImplementationEndpointName; | ||
|
|
||
| protected override bool PreferCSharpOverHtmlIfPossible => true; |
There was a problem hiding this comment.
This strictly out of scope, and was an oversight from a previous PR that turned this on for Go To Def that I just noticed, but seemed simple enough to fix.
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
Love the testing and explanations, but the design seems a little bit clunky. Is there a way we can make this more isolated, or generalize more? In the base it seems VERY targeted right now
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AbstractRazorDelegatingEndpoint.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AbstractRazorDelegatingEndpoint.cs
Outdated
Show resolved
Hide resolved
|
@ryzngard Check out the latest commit, I moved the caret projection logic into a different class, and allowed specific endpoints to provide their preferred strategy. In future we could also move the |
|
ping @dotnet/razor-tooling for reviews :D |
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AbstractRazorDelegatingEndpoint.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorSyntaxFacts.cs
Show resolved
Hide resolved
...icrosoft.AspNetCore.Razor.LanguageServer.Test/Definition/DefinitionEndpointDelegationTest.cs
Show resolved
Hide resolved
2 participants
Fixes #8536