Fixed complex trivia tab indentation #10748
Conversation
|
Thanks for the submission @michalhosala. Pinging @roslyn-ide, @CyrusNajmabadi @basoundr @heejaechang for review. |
|
@dotnet-bot test eta please, test vsi please. |
|
@michalhosala Could you please add some unit tests? You can add these unit tests inside of |
|
@basoundr unit test added, please review the latest commit in this branch. |
| @@ -81,6 +81,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Formatting | |||
|
|
|||
| Debug.Assert(Me.SecondTokenIsFirstTokenOnLine OrElse beginningOfNewLine) | |||
|
|
|||
| If Me.OptionSet.GetOption(FormattingOptions.UseTabs, LanguageNames.VisualBasic) Then | |||
There was a problem hiding this comment.
This fix solves the issue but I think the correct location to provide the fix would be, inside of CodeShapeAnalyzer.OnWhiteSpace(trivia, index) which is eventually called by CodeShapeAnalyzer.ShouldFormatMultiLine. With the current fix, we will end up formatting the Complex trivia every time we see the UseTabs is enabled irrespective of the content of the trivia. ComplexTrivia formatting is an expensive operation and it should not be done all the time.
There was a problem hiding this comment.
That's entirely possible but as I said in my original comment, the fix is copied over from Microsoft.CodeAnalysis.CSharp.Formatting.TriviaDataFactory.ComplexTrivia.ShouldFormat and I believe the person writing those lines had better understanding of the code than I so I reused them.
Therefore the solution I am providing is entirely up to performance standards set up by current c# complex trivia formatter and ultimately roslyn itself. To improve the performance, feature request should be started.
|
👍 |
|
@dotnet-bot retest vsi please |
|
@Pilchie any way I can help further in order to get this pull request merged? |
|
👍 Thanks for bringing it back to my attention! Let's do a retest of it against the current state, and then we'll merge. @dotnet-bot retest this please |
|
retest roslyn_prtest_win_vsi0 please |
|
Trying another magic string: |
|
Okay - looks good (don't work about eta2 - it was an interim thing that doesn't exist anymore, but you can't remove statuses from github). |
|
Thanks for the contribution! |
Fix should address bug number #10742. Problem was with formatting of complex trivias in VB.NET only. To fix the issue, logic from
Microsoft.CodeAnalysis.CSharp.Formatting.TriviaDataFactory.ComplexTrivia.ShouldFormatmethod was copied over toMicrosoft.CodeAnalysis.VisualBasic.Formatting.TriviaDataFactory.ComplexTrivia.ShouldFormat.