Increase indent level after line end with certain keywords #3196

xuanduc987 · 2017-06-09T06:44:56Z

No unit test because I don't know how to assert these indent level :(

msftclas · 2017-06-09T06:45:00Z

@xuanduc987,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

dsyme · 2017-06-09T12:49:38Z

Some test failurs in part1:

1) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.IndentationServiceTests.TestIndentation(Some(0),6,"\r\n// Learn more about F# at http://fsharp.org\r\n// See the 'F# Tutorial' project for more help.\r\n\r\n[<EntryPoint>]\r\nlet main argv = \r\n    printfn \"%A\" argv\r\n    0 // return an integer exit code")
  Indentation on line 6 doesn't match
  Expected: 0
  But was:  4
at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.IndentationServiceTests.TestIndentation(FSharpOption`1 expectedIndentation, Int32 lineNumber, String template) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\IndentationServiceTests.fs:line 69

2) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.IndentationServiceTests.TestIndentation(Some(0),4,"\r\nnamespace ProjectNamespace\r\n\r\ntype Class1() = \r\n    member this.X = \"F#\"")
  Indentation on line 4 doesn't match
  Expected: 0
  But was:  4
at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.IndentationServiceTests.TestIndentation(FSharpOption`1 expectedIndentation, Int32 lineNumber, String template) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\IndentationServiceTests.fs:line 69

dsyme · 2017-06-09T12:49:55Z

Adjusting the tests should be adequate test coverage

xuanduc987 · 2017-06-09T14:09:54Z

🤦‍♂️
I was looking for test with trait LanguageService.

saul · 2017-06-09T14:49:14Z

Shouldn't we be using the tokenizer here instead of regexes to match keywords?

saul · 2017-06-09T14:49:56Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

+                      )
+                      \s*$"
+                if Regex.IsMatch(previousLine.ToString(), regex, RegexOptions.IgnorePatternWhitespace) then
+                    Some ((lastIndent/tabSize + 1) * tabSize)


Isn't this just Some (lastIndent + tabSize)?

Logically, yes, technically no, if the indent is not a multiple of tabSize.

> let lastIndent = 3;; val lastIndent : int = 3 > let tabSize = 4;; val tabSize : int = 4 > (lastIndent/tabSize + 1) * tabSize;; val it : int = 4 > lastIndent + tabSize;; val it : int = 7

It's consistent with L42 above

KevinRansom

Nice.

KevinRansom · 2017-06-09T17:41:44Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

+                       | begin | do | class | function | then | else | struct | try
+                      )
+                      \s*$"
+                if Regex.IsMatch(previousLine.ToString(), regex, RegexOptions.IgnorePatternWhitespace) then


@xuanduc987

Hi,
@brettfo just pointed out that lines ending with comments may give an issue, here.
both for single line comments // and comment blocks (*

What do you propose we in those circumstances?

Kevin

@KevinRansom
A better approach should be like @saul said, using tokenizer but I honestly don't know how.
I think comments end with the above keywords are not as common as normal code though.

saul · 2017-06-13T10:31:29Z

@xuanduc987 use Tokenizer.tokenizeLine. See the ImplementInterfaceCodeFixProvider.fs file for example usage: https://github.com/Microsoft/visualfsharp/blob/6f5b2c4d78ab386a1b03a65457bdc7f8561635d0/vsintegration/src/FSharp.Editor/CodeFix/ImplementInterfaceCodeFixProvider.fs#L150

Hopefully that's enough to get you started :)

xuanduc987 · 2017-06-13T13:05:22Z

@saul
Thank you, I had looked at it earlier but was discouraged to see it depends on ProjectInfoManager, and it's async and what not.
I will take a stab at it again later :)

saul · 2017-06-13T14:33:59Z

@xuanduc987 there's no reason why you can't change us from ISynchronousIndentationService to IIndentationService. They have the same API (apart from the latter returning a Task)

xuanduc987 · 2017-06-14T05:08:10Z

@saul Thanks for the pointer, I had push some commits to use Tokenizer.
It would be a huge help if you can review it :)

saul

This is looking good :) I have a couple of questions though:

Does this also indent after the following code?

let foo () = <enter>

Or:

{ new MyInterface with <enter>

Also please can write some unit tests for the cases that you've added? This looks fantastic so far

saul · 2017-06-14T07:40:46Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

+                        match lastToken with
+                        | NeedIndent -> (lastIndent/tabSize + 1) * tabSize
+                        | _ -> lastIndent
+        }

    interface ISynchronousIndentationService with


Can we use IIndentationService instead?

I don't know the different but seems like C# and VB also use ISynchronousIndentationService

saul · 2017-06-14T07:51:45Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

+                       loop xs
+                   else Some x
+
+           return! loop (List.rev tokens)


You can get rid of the loop function and this can just become tokens |> List.rev |> List.tryFind (fun x -> x.Tag <> FSharpTokenTag.WHITESPACE)

saul · 2017-06-14T07:53:01Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

+
+        maybe {
+            // No indentation on the first line of a document
+            if lineNumber = 0 then return! None


We only need the else branch of this if statement - tryFindPreviousNonEmptyLine already checks if lineNumber is 0.

saul · 2017-06-14T10:02:17Z

vsintegration/src/FSharp.Editor/Formatting/IndentationService.fs

-                        | _ -> spaces
-                Some (loop 0 0)
+
+        let rec tryFindLastNoneEmptyToken (line: TextLine) = maybe {


Maybe rename this from tryFindLastNoneEmptyToken to tryFindLastNonWhitespaceToken

saul · 2017-06-14T10:03:30Z

@KevinRansom if @xuanduc987 is happy with this I think this is good to merge 👍

xuanduc987 · 2017-06-14T11:56:07Z

@dotnet-bot test Windows_NT Release_ci_part3 Build please

xuanduc987 · 2017-06-14T12:36:20Z

@saul @KevinRansom All green 💯

saul · 2017-06-14T13:00:31Z

vsintegration/tests/unittests/IndentationServiceTests.fs

+try
+    failwith \"fail\"
+with
+    | :? System.Exception -> \"error\"


We aren't testing whether this line gets indented.

Also these test cases would be far easier to read if instead of specifying which lines we expect indentation in a separate array, we specified them inline in the template.

For example, these few lines would become:

try failwith \"fail\" // $Indent: 4$ with | :? System.Exception -> \"error\" // $Indent: 4$

You could then parse the template like:

let indentComment = System.Text.RegularExpressions.Regex(@"\/\/\s*\$\s*Indent:\s*(\d+)\s*\$") template.Split [|'\n'|] |> Seq.map (fun s -> s.Trim()) |> Seq.indexed |> Seq.choose (fun (line, text) -> let m = indentComment.Match text if m.Success then Some (line, System.Convert.ToInt32 m.Groups.[1].Value) else None )

This will return you a list of lineNumber * indentSize.

Thank you.
As for the line after with, it would not be auto indented, because match x with <enter> should not.

saul · 2017-06-14T14:55:21Z

Fantastic 😃

* Increase indent level after line end with certain keywords * Fix test case * Use Tokenizer * Refactoring and fix test * Saul review * Add more test * Fix tests * Rename tryFindLastNoneEmptyToken -> tryFindLastNoneWhitespaceToken * Trailing comment is ok * Move auto indent to a separate test * Use same indentation as the previous comment line

msftclas added the cla-already-signed label Jun 9, 2017

xuanduc987 added 2 commits June 9, 2017 22:21

Increase indent level after line end with certain keywords

e781036

Fix test case

243f0ee

xuanduc987 force-pushed the smarter-indent branch from c53aee5 to 243f0ee Compare June 9, 2017 14:06

saul reviewed Jun 9, 2017

View reviewed changes

KevinRansom approved these changes Jun 9, 2017

View reviewed changes

KevinRansom reviewed Jun 9, 2017

View reviewed changes

xuanduc987 added 2 commits June 14, 2017 12:09

Use Tokenizer

aa5b82c

Refactoring and fix test

26b3f6d

saul reviewed Jun 14, 2017

View reviewed changes

xuanduc987 added 2 commits June 14, 2017 17:46

Saul review

ac72293

Add more test

9fa4250

saul reviewed Jun 14, 2017

View reviewed changes

saul approved these changes Jun 14, 2017

View reviewed changes

xuanduc987 added 2 commits June 14, 2017 20:20

Fix tests

2129896

Rename tryFindLastNoneEmptyToken -> tryFindLastNoneWhitespaceToken

5f0290d

saul reviewed Jun 14, 2017

View reviewed changes

xuanduc987 added 2 commits June 14, 2017 22:54

Trailing comment is ok

16cb6ad

Move auto indent to a separate test

9838ea4

Use same indentation as the previous comment line

bb642ca

dsyme approved these changes Jun 15, 2017

View reviewed changes

KevinRansom merged commit 4ed4eb0 into dotnet:master Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase indent level after line end with certain keywords #3196

Increase indent level after line end with certain keywords #3196

xuanduc987 commented Jun 9, 2017

msftclas commented Jun 9, 2017

dsyme commented Jun 9, 2017

dsyme commented Jun 9, 2017

xuanduc987 commented Jun 9, 2017

saul commented Jun 9, 2017

saul Jun 9, 2017

0x53A Jun 9, 2017

xuanduc987 Jun 9, 2017

KevinRansom left a comment

KevinRansom Jun 9, 2017

xuanduc987 Jun 10, 2017

saul commented Jun 13, 2017

xuanduc987 commented Jun 13, 2017

saul commented Jun 13, 2017 •

edited

Loading

xuanduc987 commented Jun 14, 2017

saul left a comment

saul Jun 14, 2017

xuanduc987 Jun 14, 2017

saul Jun 14, 2017 •

edited

Loading

saul Jun 14, 2017

saul Jun 14, 2017

saul commented Jun 14, 2017 •

edited

Loading

xuanduc987 commented Jun 14, 2017

xuanduc987 commented Jun 14, 2017

saul Jun 14, 2017

xuanduc987 Jun 14, 2017 •

edited

Loading

saul commented Jun 14, 2017

Increase indent level after line end with certain keywords #3196

Increase indent level after line end with certain keywords #3196

Conversation

xuanduc987 commented Jun 9, 2017

msftclas commented Jun 9, 2017

dsyme commented Jun 9, 2017

dsyme commented Jun 9, 2017

xuanduc987 commented Jun 9, 2017

saul commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saul commented Jun 13, 2017

xuanduc987 commented Jun 13, 2017

saul commented Jun 13, 2017 • edited Loading

xuanduc987 commented Jun 14, 2017

saul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saul Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saul commented Jun 14, 2017 • edited Loading

xuanduc987 commented Jun 14, 2017

xuanduc987 commented Jun 14, 2017

Choose a reason for hiding this comment

xuanduc987 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

saul commented Jun 14, 2017

saul commented Jun 13, 2017 •

edited

Loading

saul Jun 14, 2017 •

edited

Loading

saul commented Jun 14, 2017 •

edited

Loading

xuanduc987 Jun 14, 2017 •

edited

Loading