Skip to content

Make matched brace highlighting work exactly as in C# editor#5049

Merged
brettfo merged 7 commits into
dotnet:masterfrom
vasily-kirichenko:fix-matching-brace-highlighting
Jun 5, 2018
Merged

Make matched brace highlighting work exactly as in C# editor#5049
brettfo merged 7 commits into
dotnet:masterfrom
vasily-kirichenko:fix-matching-brace-highlighting

Conversation

@vasily-kirichenko

Copy link
Copy Markdown
Contributor

This fixes #3794

F#

_1

C#

_1

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

@dotnet-bot test Ubuntu16.04 Release_default Build please

@cartermp

Copy link
Copy Markdown
Contributor

I tried this in #3849 - unfortunately, it broke smart auto de-indentation (#3313). FWIW, I think that it's the correct way to do it as you've done here.

Perhaps the editor formatting feature could be adjusted to call a different routine (GetBraceMatchingResultAlternate?) that does this:

let length = position - range.Start
length >= 0 && length <= range.Length

And we keep this so that we correctly match braces when there is nesting.

It's also worth double checking that what's raised in #2092 is still addressed (or at least that we think it's reasonable that some of that isn't addressed).

@saul

saul commented May 31, 2018

Copy link
Copy Markdown
Contributor

Yeah... pretty sure the Roslyn tests aren't running so the auto-deindent tests also won't be running. Please can you verify that they pass on your end?

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

@saul I stumbled upon missing Test attributes at
https://github.com/Microsoft/visualfsharp/pull/5049/files#diff-29de5ba2f239f0087e2e608aad8c4d7bR173 and added them. Maybe they were turned off intentionally?

@saul saul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I’m not at a computer so I can’t check blame - I definitely ran then when I wrote them so at some point they must have had the attribute

@cartermp

Copy link
Copy Markdown
Contributor

#3601 had test attributes removed, but they were not previously [<Test>]...

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

Locally the tests fail

image

@cartermp

Copy link
Copy Markdown
Contributor

@KevinRansom is looking at these failures (I can reproduce them locally). There is an underlying assumption that a particular VS .dll is present for these tests to execute, and that .dll is not there (nor is it possible to guarantee that it is there)

@saul

saul commented May 31, 2018

Copy link
Copy Markdown
Contributor

Have you pulled @cartermp’s change?

@cartermp

Copy link
Copy Markdown
Contributor

Hmm, looks like you did get other tests to pass...

@smoothdeveloper

Copy link
Copy Markdown
Contributor

there is an extra test commented out there:

4c244da#diff-d191e5cda6609ca754424549217cdcf1

@auduchinok

Copy link
Copy Markdown
Member

Yep, 4c244da#diff-2c4d428f853538f7a2c0cbc29874ec19L170
@auduchinok !

If I remember correctly @dsyme has been fixing the VS integration tests back then as they were failing in the PR.

@KevinRansom

Copy link
Copy Markdown
Contributor

@auduchinok it's me. I'm on the hook for fixing the tests.

@brettfo

brettfo commented Jun 5, 2018

Copy link
Copy Markdown
Member

@KevinRansom I took a look at these changes and the test failures mentioned above pass for me:

NUnit Console Runner 3.0.5797
Copyright (C) 2015 Charlie Poole

Runtime Environment
   OS Version: Microsoft Windows NT 10.0.17134.0
  CLR Version: 4.0.30319.42000

Test Files
    D:\Microsoft\visualfsharp2\debug\net40\bin\VisualFSharp.UnitTests.dll

Test Filters
    Test: Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.EditorFormattingServiceTests.TestIndentation


Run Settings
    ProcessModel: InProcess
    RuntimeFramework: V4.0
    RunAsX86: True
    WorkDirectory: D:\Microsoft\visualfsharp2\debug\net40\bin
    Verbose: True
    NumberOfTestWorkers: 8

Test Run Summary
    Overall result: Passed
   Tests run: 4, Passed: 4, Errors: 0, Failures: 0, Inconclusive: 0
     Not run: 0, Invalid: 0, Ignored: 0, Explicit: 0, Skipped: 0
  Start time: 2018-06-05 22:12:40Z
    End time: 2018-06-05 22:12:42Z
    Duration: 2.079 seconds

Results (nunit3) saved as TestResult.xml
Press any key to continue . . .

@cartermp cartermp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as per:

  • The code handling things correctly
  • It works when I use it

@brettfo brettfo merged commit e6c2a38 into dotnet:master Jun 5, 2018
@cartermp

cartermp commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

Confirming that this fixes #4274 as well

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…5049)

* make matched brace highlighting work exactly as in C# editor

* fix tests

* Add optional param for formatting

* revert matching braces logic in FSharpEditorFormattingService

* add missing test attributes

* Revert "revert matching braces logic in FSharpEditorFormattingService"

This reverts commit 5ddaba3.
T-Gro pushed a commit that referenced this pull request Jun 12, 2026
* make matched brace highlighting work exactly as in C# editor

* fix tests

* Add optional param for formatting

* revert matching braces logic in FSharpEditorFormattingService

* add missing test attributes

* Revert "revert matching braces logic in FSharpEditorFormattingService"

This reverts commit 5ddaba3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Outer parens are not highlighted

7 participants