Show TODO Razor comments in the Task List in VS #11540
Conversation
# Conflicts: # src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostDocumentPullDiagnosticsEndpoint.cs # src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/CohostDocumentPullDiagnosticsTest.cs
|
Test insertion to make sure I don't annoy RPS: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/614387 |
There was a problem hiding this comment.
Generally looks good! A couple of points:
- Consider whether you want to add a new integration test. I think this feature probably warrants one to verify that the TODO comments actually show up in VS Task List.
- It looks like the test insertion uncovered a regression in non-devdiv methods JIT'd. I'm not sure how that could happen from this PR, but it looks like the regression was in all five iterations of the run.
| @@ -60,4 +73,37 @@ private static FormattingFlags GetFormattingFlags(ClientSettings settings) | |||
|
|
|||
| return flags; | |||
| } | |||
|
|
|||
| public virtual bool Equals(RazorLSPOptions? other) | |||
There was a problem hiding this comment.
Do you intend for new records to be derived from RazorLSPOptions? If not, please mark RazorLSPOptions as sealed. If you do want RazorLSPOptions to be subtyped, add a check here to compare EqualityContract and add it to `GetHashCode() as well. Otherwise, equality between subtypes will be incorrect.
| @@ -60,4 +73,37 @@ private static FormattingFlags GetFormattingFlags(ClientSettings settings) | |||
|
|
|||
| return flags; | |||
| } | |||
|
|
|||
| public virtual bool Equals(RazorLSPOptions? other) | |||
There was a problem hiding this comment.
Why is this virtual rather than override?
| if (i + token.Length + 2 > comment.EndCommentStar.SpanStart || // Enough room in the comment for the token and some content? | ||
| !Matches(source, i, token) || // Does the prefix match? | ||
| char.IsLetter(source[i + token.Length + 1])) // Is there something after the prefix, so we don't match "TODOLOL" |
There was a problem hiding this comment.
It feels like the first and last checks could be pulled into Matches. At the very least, the last check could be moved after the loop in Matches so that all of the character tests are in one place.
| ErrorHandler.ThrowOnFailure(taskListService.EnumTokens(out var enumerator)); | ||
| ErrorHandler.ThrowOnFailure(enumerator.Next((uint)count, tokens, out var numFetched)); | ||
|
|
||
| using var tokensBuilder = new PooledArrayBuilder<string>(); |
There was a problem hiding this comment.
Since the size is known ahead of tiume, pass count to PooledArrayBuilder<string>().
| public ClientAdvancedSettings GetAdvancedSettings() => new(FormatOnType, | ||
| AutoClosingTags, | ||
| AutoInsertAttributeQuotes, | ||
| ColorBackground, | ||
| CodeBlockBraceOnNextLine, | ||
| CommitElementsWithSpace, | ||
| Snippets, | ||
| LogLevel, | ||
| FormatOnPaste, | ||
| TaskListDescriptors); |
There was a problem hiding this comment.
Nit: When expression bodies get large, I prefer to just move the => to the next line. That would probably have been enough for this case rather than breaking the arguments onto separate lines.
| Assert.Equal(2, serverCapabilities.DiagnosticProvider.DiagnosticKinds.Length); | ||
|
|
||
| // use the expected value directly; if the underlying library changes values, there is likely a downstream impact | ||
| Assert.Equal("syntax", serverCapabilities.DiagnosticProvider.DiagnosticKinds[0].Value); | ||
| Assert.Equal("task", serverCapabilities.DiagnosticProvider.DiagnosticKinds[1].Value); |
There was a problem hiding this comment.
Consider switchin this over to Assert.Collection(...)
| @@ -23,7 +23,7 @@ public RazorLSPOptionsMonitorTest(ITestOutputHelper testOutput) | |||
| public async Task UpdateAsync_Invokes_OnChangeRegistration() | |||
| { | |||
| // Arrange | |||
| var expectedOptions = new RazorLSPOptions(FormattingFlags.Disabled, AutoClosingTags: true, InsertSpaces: true, TabSize: 4, AutoShowCompletion: true, AutoListParams: true, AutoInsertAttributeQuotes: true, ColorBackground: false, CodeBlockBraceOnNextLine: false, CommitElementsWithSpace: true); | |||
| var expectedOptions = new RazorLSPOptions(FormattingFlags.Disabled, AutoClosingTags: true, InsertSpaces: true, TabSize: 4, AutoShowCompletion: true, AutoListParams: true, AutoInsertAttributeQuotes: true, ColorBackground: false, CodeBlockBraceOnNextLine: false, CommitElementsWithSpace: true, TaskListDescriptors: []); | |||
There was a problem hiding this comment.
Please wrap these and share the same RazorLSPOptions instance across tests that have the same values. (All are identical in this diff.)
| @@ -82,7 +82,7 @@ public void Update_TriggersChangedIfAdvancedSettingsAreDifferent() | |||
| var manager = new ClientSettingsManager(_clientSettingsChangeTriggers); | |||
| var called = false; | |||
| manager.ClientSettingsChanged += (caller, args) => called = true; | |||
| var settings = new ClientAdvancedSettings(FormatOnType: false, AutoClosingTags: true, AutoInsertAttributeQuotes: true, ColorBackground: true, CodeBlockBraceOnNextLine: false, CommitElementsWithSpace: false, SnippetSetting: default, LogLevel: default, FormatOnPaste: false); | |||
| var settings = new ClientAdvancedSettings(FormatOnType: false, AutoClosingTags: true, AutoInsertAttributeQuotes: true, ColorBackground: true, CodeBlockBraceOnNextLine: false, CommitElementsWithSpace: false, SnippetSetting: default, LogLevel: default, FormatOnPaste: false, TaskListDescriptors: []); | |||
There was a problem hiding this comment.
Please wrap the arguments. Also, you can use 0 instead of default for the enum values at the end to shorten it a bit.
|
The RPS and Speedometer regressions are also present in our main branch insertions and my System.Text.JSON test insertions, so whatever they are, I'm confident they're not caused by this change. |
Awesome! Adding my approval then, though do consider whether an integration test for the end-to-end would be valuable. |
Fixes #4446