Needs Design Review: First pass at fixing the formatting of VB Line Continuation Formatting by paul1956 · Pull Request #38072 · dotnet/roslyn

paul1956 · 2019-08-17T06:34:43Z

This addresses _ on line without code, with or without comment. This uses existing Formatting Rules which don't understand VB Line Continuation on blank lines or followed by comment. The results need a design review. I temporarily renamed the effected tests by added 3 _, they could be removed but it would be harder to see the effects of any changes, these are the only tests that I found that hit the code I changed. One area that needs review is the alignment of lines under lines with _ comment.

…ent. This addresses _ on line without code with out without comment. This uses existing Formatting Rules which don't understand VB Line Continuation on blank lines or followed by comment.

CyrusNajmabadi · 2019-08-18T23:02:16Z

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

-                 2 + _
-                 3
+                     2 + _
+                         3


this formatting doesn't seem desirable.

It is not desirable and I would love to fix it but I don't see a rule that would format it correctly and doesn't undo all the newer formatting. I have experimented with the available rules and you either get this behavior or the "2" is left aligned sometimes and correctly aligned others depending on what the line looks like before the formatting.

The code that does the indenting based on the rules needs to understand " _ ' Optional Comment' and I have not found it yet or I need to use a different rule.

I would love to fix it but I don't see a rule that would format it correctly and doesn't undo all the newer formatting

What do you mean by 'newer formatting'?

The code that does the indenting based on the rules needs to understand " _ ' Optional Comment' and I have not found it yet or I need to use a different rule.

Can you write a set of tests that show the behavior you want roslyn to have, but doesn't have? That way we can then have a code change that makes those tests pass, while not regressing these other scenarios?

That is my goal, I restored all the original tests, some fail is expected other fail because my code incorrectly breaks them. Is there a way to expect the old test to fail and have it pass?

CyrusNajmabadi · 2019-08-19T02:26:10Z

they could be removed but it would be harder to see the effects of any changes,

Please don't do this. It appears as if your code changes are regressing these scenarios. Removing tests will simply hide that and allow regressions in areas we have tests explicitly there to ensure the behavior of.

these are the only tests that I found that hit the code I changed. One area that needs review is the alignment of lines under lines with _ comment.

I think you need to update your change so that it can give you the behavior you want without regressing these cases.

CyrusNajmabadi · 2019-08-19T02:27:17Z

I don't see any tests actually showing cases you think should be fixed/improved by your product code change. Can you write tests that at least show what positive behavior you think the changes so far have?

CyrusNajmabadi · 2019-08-19T02:27:52Z

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/VisualBasicTriviaFormatter.vb

            If trivia2.Kind = SyntaxKind.LineContinuationTrivia Then
-                Return LineColumnRule.ForceSpacesOrUseAbsoluteIndentation(spacesOrIndentation:=1)
+                If trivia1.Kind = SyntaxKind.None Then
+                    Return LineColumnRule.ForceSpacesOrUseAbsoluteIndentation(spacesOrIndentation:=1)


what code example causes this case to be hit?

A file with only _

please document this code to make those cases clear.

CyrusNajmabadi · 2019-08-19T02:29:37Z

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/VisualBasicTriviaFormatter.vb

+                ElseIf trivia1.Kind = SyntaxKind.commenttrivia Then
+                    Return LineColumnRule.ForceSpacesOrUseDefaultIndentation(spaces:=-1)
+                Else
+                    Return LineColumnRule.PreserveLinesWithFollowingPrecedingIndentation()


can you provide comments in this code giving examples of what each of these if-blocks would correspond to and why the LineColumnRules you are returning are desirable? For example, i have no idea why you are returning spacesOrIndentation:=1 for trivia1.Kind = SyntaxKind.None or why you return spaces:=-1 for trivia1.Kind = SyntaxKind.commenttrivia. etc. etc.

Done in newest commit

CyrusNajmabadi · 2019-08-19T02:32:26Z

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

                    Dim aa = 1 + _
            _
-                                          _
+                                          _' Comment 1


please don't change the inputs of existing tests. Instead, add a new test with the different input. By doing this, i can't see what the impact of your code change is on hte original input of the original test.

Yes I realized I needed to do that. Some of the old tests will fail but I think for now that is OK.

CyrusNajmabadi · 2019-08-19T02:33:23Z

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

        <Fact, Trait(Traits.Feature, Traits.Features.Formatting)>
        <WorkItem(529899, "DevDiv_Projects/Roslyn")>
-        Public Async Function IndentContinuedLineOfSingleLineLambdaToFunctionKeyword() As Task
+        Public Async Function IndentContinuedLineOfSingleLineLambdaToFunctionKeyword___() As Task


why did you add ___ to all the tests?

So I could identify all the tests that are effected by my changes, as I said it was temporary. I will put the old tests back (which was always my intent) and put my changes next to them with ___.

I have restored all the old test, there are 3 possibilities, New ___ tests passes and old test fails this is expected, the new test is what I think the code should look like. Both tests fail, here the old test is expected to fail but my changes did not produce desired result as shown in new test. Old test fails and there is no new test, my changes broke the old code.

…e results should be. When an old test fails and new test passes this is the correct new behaviour. If the old test fails and there is no new test I broke the old test and need guidance, if both fail the desited result is the new test. VisualBasicTriviaFormatter only fixed case error no logic changes.

paul1956 · 2019-08-19T16:26:22Z

The main thing I am trying to change is what happens when a _ with optional Comment is on a line by itself and what happens on the code line following after that line(s). Today lines after, align left with 1 space. This is similer to how in VB aligns Directives Vs. C#. All of my attempts to date have broken the code followed by _ ' Comment on a single line case or caused the newly indented lines to drift right by two space when a comment is present.

CyrusNajmabadi · 2019-08-19T18:13:10Z

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/VisualBasicTriviaFormatter.vb

+                ElseIf trivia1.Kind = SyntaxKind.CommentTrivia Then
+                    ' _ ' Comment aligns with the ' instead of the _ causing each line to move right
+                    ' 2 space
+                    Return LineColumnRule.ForceSpacesOrUseDefaultIndentation(spaces:=-1)


so, in terms of a comment, this isn't really helpful. What i'd like is both an example of waht the original VB code looks like that this case is trying to handle, what we want the final code to look like, and why this particular method/arguent is the right thing to do that. i.e. why do we use ForceSpacesOrUseDefaultIndentation? why do we pass spaces:=-1 to it?

I honestly don't know the answers, and reviewing the PR isn't informative enough. As such, the code needs effective comments to make it clear.

CyrusNajmabadi · 2019-08-19T18:13:24Z

Tagging @heejaechang . @heejaechang can you help @paul1956 here?

paul1956 · 2020-01-04T10:24:39Z

Ping @sharwell, I started looking at this again and if I can't get some help I should close. It seems that since I started working on this new code fixes and tests have been added that depend on the current behavior of _ and they break with my PR and it is unclear to me if I need to fix their tests (becuase my new formatting is better) or my code (because the current formatting is better/desired) and as has been said the behavior of my code is sometimes better but sometimes odd or undesirable.

… tests that break with changes or don't do the desired thing with 3 _, if there is a newer test I added use 4 _. All the _ renaming is temporary to make it easy to identify affected tests.

…anged

sharwell · 2020-05-14T20:32:42Z

I added several tests for the current state of line continuation support in #44267. I'm not sure we need to implement handling beyond this, but I at least wanted to capture the test cases. 👍

First pass at fixing the formatting of VB Line Continuation with comm…

0ee0b1b

…ent. This addresses _ on line without code with out without comment. This uses existing Formatting Rules which don't understand VB Line Continuation on blank lines or followed by comment.

paul1956 requested a review from a team as a code owner August 17, 2019 06:34

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 17, 2019

jinujoseph assigned sharwell Aug 17, 2019

CyrusNajmabadi reviewed Aug 18, 2019

View reviewed changes

CyrusNajmabadi reviewed Aug 19, 2019

View reviewed changes

paul1956 added 2 commits August 18, 2019 21:53

Add comment for extra If branches, no logic changes

c013b2a

CyrusNajmabadi reviewed Aug 19, 2019

View reviewed changes

Deleted wrong test, TestBang should pass.

663f8e3

sharwell added the Need Design Review The end user experience design needs to be reviewed and approved. label Jan 10, 2020

paul1956 added 2 commits January 15, 2020 19:08

Restore formating logic to before changes (using conditional), rename…

6acfb90

… tests that break with changes or don't do the desired thing with 3 _, if there is a newer test I added use 4 _. All the _ renaming is temporary to make it easy to identify affected tests.

Set #Const LineContinueCommentSupport = False to restore code to unch…

d4c1923

…anged

sharwell mentioned this pull request May 14, 2020

Add formatting tests for comments after line continuation #44267

Merged

sharwell closed this May 14, 2020

sharwell added Resolution-Expired The request is closed due to inactivity under our contribution review policy. and removed Need Design Review The end user experience design needs to be reviewed and approved. labels May 14, 2020

-+ _
-
++ _
+

Conversation

paul1956 commented Aug 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 19, 2019

Uh oh!

CyrusNajmabadi commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul1956 Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul1956 Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul1956 commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 19, 2019

Uh oh!

paul1956 commented Jan 4, 2020

Uh oh!

sharwell commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi Aug 19, 2019 •

edited

Loading

paul1956 Aug 19, 2019 •

edited

Loading

paul1956 Aug 19, 2019 •

edited

Loading