Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

vasily-kirichenko
Copy link
Contributor

This fixes #3794

F#

_1

C#

_1

@vasily-kirichenko
Copy link
Contributor Author

@dotnet-bot test Ubuntu16.04 Release_default Build please

@cartermp
Copy link
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
Copy link
Contributor

saul commented May 31, 2018

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
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?

Copy link
Contributor

@saul saul left a comment

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
Contributor

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

@vasily-kirichenko
Copy link
Contributor Author

@vasily-kirichenko
Copy link
Contributor Author

Locally the tests fail

image

@cartermp
Copy link
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
Copy link
Contributor

saul commented May 31, 2018

Have you pulled @cartermp’s change?

@cartermp
Copy link
Contributor

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

@smoothdeveloper
Copy link
Contributor

there is an extra test commented out there:

4c244da#diff-d191e5cda6609ca754424549217cdcf1

@auduchinok
Copy link
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
Member

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

@brettfo
Copy link
Member

brettfo commented Jun 5, 2018

@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 . . .

Copy link
Contributor

@cartermp cartermp left a comment

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
Copy link
Contributor

cartermp commented Jun 5, 2018

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.
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