Add editor formatting service to auto-deindent closing brackets #3313

saul · 2017-07-07T19:49:09Z

At no point in this GIF do I Shift-Tab to de-indent. Closing braces automatically deindent to the same indentation level as that of the opening brace.

It performs the de-indent check on Enter, and when one of ), ] or } is pressed.

…tion'

cartermp

This is great. Just have two suggestions. Not sure if the scenario implied by them is realistic though.

cartermp · 2017-07-08T04:55:18Z

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

+        asyncMaybe {
+            let! options = optionsOpt
+
+            let line = sourceText.Lines.[sourceText.Lines.IndexOf position]


Not sure if this is realistic or would ever get hit, but max 0 (sourceText.Lines.IndexOf position) would be useful.

Perhaps this is just me being paranoid after #3180.

IndexOf throws if the position isn't within the text, but as Roslyn has already supplied us with the position I think it's impossible that we can't find the line in the source text (also supplied by Roslyn).

cartermp · 2017-07-08T04:57:15Z

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

+                    |> Seq.takeWhile ((=) ' ')
+                    |> Seq.length
+
+                let startIndent = indentChars sourceText.Lines.[sourceText.Lines.IndexOf leftSpan.Start]


Same as above, or perhaps check this before hand if a startIndent of 0 would cause trouble?

Tested with startIndent 0 and everything's fine :)

cartermp · 2017-07-08T05:02:20Z

Also, perhaps making this be driven by an editor option (and on by default) would also be good if that's possible. I imagine some people would like to turn it off if they prefer a more plain editor.

saul · 2017-07-08T10:32:02Z

Phillip I've set it so that smart indentation is only done if the indentation style is set to 'Smart' in the editor settings. However I think by default it's set to 'Block' and I think we should move everyone over to this style by default - any thoughts as to how we could do this?

vasily-kirichenko · 2017-07-08T10:39:21Z

I'm for as smart style as possible by default. After writing some Kotlin in Inellij F# VS editor feels annoyingly dumb.

cartermp · 2017-07-08T16:25:20Z

@saul This line seems like it should be setting it already; if that's not the case, then we definitely have a bug in registering the language service.

cartermp · 2017-07-08T16:34:37Z

@dotnet-bot test this please

saul · 2017-07-08T19:22:50Z

Phillip it's shown as an option but it's not the default

cartermp · 2017-07-08T19:32:50Z

Ah, I was mistaken in thinking this was what should also set it. Clearly not the case based on the docs for that. I've no idea where this would be set. Isn't there a common place where we're setting some of the newer settings which were added?

forki · 2017-07-09T08:33:41Z

This is great. Is this something that other editors can use as well? /cc @Krzysztof-Cieslak

forki · 2017-07-09T08:36:22Z

Ok everything is in vsintegration. So that's a no. ;-(
Anyway: cool feature. Thanks a lot

vasily-kirichenko · 2017-07-09T08:46:42Z

I'm still waiting when anything from Code can be used in VS.

forki · 2017-07-09T08:50:48Z

Most of the fable, react, react native tooling is develop in code. That stuff will flow to VS in vnext. Also a lot of the paket / netcore integration stuff. It's all done so that all editors benefit

Krzysztof-Cieslak · 2017-07-09T18:21:22Z

I'm still waiting when anything from Code can be used in VS.

I'm still waiting for features be implemented in FCS instead of being coupled with VS. At least in our case features are shared between xplat editors using FSAC.

dsyme · 2017-07-10T15:57:25Z

I'm still waiting for features be implemented in FCS instead of being coupled with VS. At least in our case features are shared between xplat editors using FSAC.

@Krzysztof-Cieslak I've a WIP integration of a large amount of work into the FCS repo , where we will do final testing on FCS (which actually caught some issues), add some documentation and a release.

After this integration the whole test/release process should be easier. We will also accept contributions directly to FCS and cherry-pick them into VS if people have a strong preference to contribute them directly to FCS.

Pilchie · 2017-07-10T16:16:12Z

@jasonmalinowski do you know how to set the default value for indenting for a language service to "Smart"?

Krzysztof-Cieslak · 2017-07-11T21:54:36Z

@dsyme, I think I do understand the workflow ;) My point was about the fact that whole logic of this feature (and many others) is implemented in vsintegration layer that's not part of things ported to FCS, and is directly using Roslyn APIs which makes porting to any other editor difficult. What is totally fine, but given this fact Vasily's comment was not really fair, IMHO.

Anyway, great feature. 💯

vasily-kirichenko · 2017-07-12T07:52:13Z

I meant that VF(P)T -> Ionide should not be one way road. If you like this feature be available in Ionide, make a PR here, moving the logic to FCS. People here don't care about Ionide, just as you don't care about VS.

Krzysztof-Cieslak · 2017-07-12T08:12:12Z

People here don't care about Ionide, just as you don't care about VS.

You don't care about Ionide. Pretty sure lot of contributors and maintainers of this repo care about xplat tooling

vasily-kirichenko · 2017-07-12T08:18:43Z

OK, it's me who don't care :) Anyway, what about the PR?..

dsyme · 2017-07-12T15:59:32Z

... don't care about Ionide..

I care about Ionide, very very much. I use it all the time and am slowly trying to learn how to contribute to it.

You don't have to care about Visual Studio IDE Tools to contribute to F# bits in this repo (except to make sure they continue to compile and pass CI tests as part of your PR).

If you like this feature be available in Ionide, make a PR here, moving the logic to FCS.

I agree with this

KevinRansom · 2017-07-12T16:53:45Z

@vasily-kirichenko @Krzysztof-Cieslak @dsyme. I would love to have a plan where Ionide and VS Tools shared code. However, I am not smart enough or knowledgeable enough to figure out how that would work. Perhaps one day we will get there ... for sure Ionide is something we care about.

cartermp · 2017-07-21T05:19:26Z

FYI - asked around about how to set this as the default. Lots of folks taking vacations this time of year, but I'll follow up on that again. I'm fine with taking this for now and then creating a separate issue for Smart Indent being the default.

saul · 2017-07-21T06:06:02Z

Thanks Phillip. I'm also on vacation now so I can't get around to fixing the tests until week after next.

cartermp · 2017-07-21T15:39:19Z

Sounds good. I'll flag this for follow-up ~1.5 weeks for now. Have a good time!

saul · 2017-08-23T22:19:59Z

It's worth noting we need a way to move everyone's 'Tab Style' to 'Smart' (as C# does) - otherwise people won't get any kind of auto indentation. Note that then we'd match C#'s behaviour.

saul · 2017-08-23T22:20:09Z

cc @KevinRansom @cartermp see comment above

KevinRansom · 2017-08-23T22:35:19Z

@saul without enabling by default, discovery will be hard, so I agree it should probably be the default. However, people often dislike default changes, even when the change is useful stuff.
Lets get it in and road test this for a few months, and make sure the experience is right and switch over to having smart as the default in VS2017.5.

Does that make sense?

saul · 2017-08-23T22:39:30Z

It's not a default change per se as the current behaviour will be moved to only act that way if 'Smart' is enabled. So to the end user indentation will work exactly the same, except it will de-indent too (that's what this PR is adding).

KevinRansom · 2017-08-23T22:49:45Z

@saul I'm sorry I misunderstood the question. Making the auto de-indent "smart indent only" only seems right. I think we should consider making smart the default, when we are sure the experience is solid.

Does that help?

cartermp · 2017-08-23T23:00:54Z

The question is how we do that. It's the default for me on a fresh installation of VS, and I've never gone in to set it myself, which is why that setting is confusing to me.

The experience with this PR (and @xuanduc987's) is great, IMO. The editor feels smart.

KevinRansom · 2017-08-24T01:52:03Z

It's a VS setting thingy ... there's some way of setting defaults in the internal VS codebase. I seem to remember Lincoln working it out ages ago, in order to ensure that tabs as spaces is set. I expect Johnathan or Brett know the magic goo.

KevinRansom · 2017-08-24T01:54:01Z

@dotnet-bot Windows_NT Release_ci_part1 Build please

saul · 2017-08-24T08:10:10Z

@KevinRansom I know about the failing test cases.

Also I think there's a way that we can make Jenkins outside more readable - is there a reason why we aren't parsing the NUnit test report XML? I may open a new PR that makes reading CI failures palpable.

forki · 2017-08-24T08:13:50Z

I'm very in favor of having better build output. But usually we only fix the current issue to be displayed better. All bigger efforts to change things more fundamentally never made it in. Displaying nunit errors im CI should be a good small move. But don't forget that a big part of the test suite is not nunit. Am 24.08.2017 10:10 vorm. schrieb "Saul Rennison" <notifications@github.com

…

: @KevinRansom <https://github.com/kevinransom> I know about the failing test cases. Also I think there's a way that we can make Jenkins outside more readable - is there a reason why we aren't parsing the NUnit test report XML? I may open a new PR that makes reading CI failures palpable. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNA7M3LFORXm8FjLjstkGkz8eY4udks5sbS_jgaJpZM4ORXN8> .

realvictorprm · 2017-08-24T09:00:18Z

This looks super useful, @saul is this in such a state that I can safely install it 😝 ?

saul · 2017-08-24T11:20:21Z

Indeed it is!

…

________________________________ From: Victor Peter Rouven Müller <notifications@github.com> Sent: Thursday, August 24, 2017 10:00:27 AM To: Microsoft/visualfsharp Cc: Saul Rennison; Mention Subject: Re: [Microsoft/visualfsharp] Add editor formatting service to auto-deindent closing brackets (#3313) This looks super useful, @saul<https://github.com/saul> is this in such a state that I can safely install it 😝 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#3313 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAD8jp47-465F0jdSBx4JzoONCUHc2rCks5sbTurgaJpZM4ORXN8>.

cartermp · 2017-08-24T15:44:58Z

@dotnet-bot Windows_NT Release_ci_part1 Build please

Timeout on the CI server...

cartermp · 2017-08-25T20:39:46Z

@saul An error in the log here:

=> Tests.Service.ProjectAnalysisTests.Test Project21 whole project errors
15:56:45 Project21 error: <<<This expression was expected to have type
15:56:45     'int'    
15:56:45 but here has type
15:56:45     'string'    >>>
15:56:45 Project21 error: <<<Invalid value>>>

abelbraaksma · 2017-08-25T23:46:58Z

Thanks for this change, this and other work on Smart indent is really very welcome. As an aside, all this work smart indent fixes other, often closed-without-solution issues like #1588, #1986 (where it was still believed smart indent didn't exist, probably true at the time) and #1980.

Still WIP, other tests still broken

saul · 2017-09-02T10:29:43Z

@cartermp when this goes green it's ready to merge.

saul · 2017-09-02T12:16:06Z

@dotnet-bot please test Windows_NT Release_ci_part2

Fixes dotnet#2092

saul · 2017-09-04T14:45:21Z

@KevinRansom @cartermp this is ready to merge

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

@saul
Thanks for this, it's great.

Kevin

…et#3313) * Add editor formatting service for auto-deindent * Minor refactor of the indentation service - do not indent after 'function' * Only use smart indentation if indent style is set to 'Smart' * Fix broken unit test build * Implement review comments, fix build * Fix some broken brace matching tests Still WIP, other tests still broken * Fix failing indentation tests * Add formatting service tests * Add more brace matching tests Fixes dotnet#2092

Add editor formatting service for auto-deindent

9588eb3

msftclas added the cla-already-signed label Jul 7, 2017

vasily-kirichenko approved these changes Jul 7, 2017

View reviewed changes

Minor refactor of the indentation service - do not indent after 'func…

bbf8eae

…tion'

cartermp approved these changes Jul 8, 2017

View reviewed changes

Only use smart indentation if indent style is set to 'Smart'

a7784be

Fix broken unit test build

7661490

Implement review comments, fix build

6fb0534

Fix some broken brace matching tests

6e5debc

Still WIP, other tests still broken

saul mentioned this pull request Sep 2, 2017

Unable to implement abstract class #3540

Closed

saul added 2 commits September 2, 2017 10:51

Fix failing indentation tests

4259c7c

Add formatting service tests

4cd00d5

saul mentioned this pull request Sep 2, 2017

Brace Highlighting Should Activate at more positions #2092

Closed

Add more brace matching tests

b4781b6

Fixes dotnet#2092

KevinRansom approved these changes Sep 6, 2017

View reviewed changes

cartermp merged commit a6cfc81 into dotnet:master Sep 6, 2017

cartermp mentioned this pull request May 31, 2018

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

Merged

Add editor formatting service to auto-deindent closing brackets #3313

Add editor formatting service to auto-deindent closing brackets #3313

Conversation

saul commented Jul 7, 2017 • edited Loading

cartermp left a comment

Choose a reason for hiding this comment

cartermp Jul 8, 2017

Choose a reason for hiding this comment

saul Jul 8, 2017

Choose a reason for hiding this comment

cartermp Jul 8, 2017

Choose a reason for hiding this comment

saul Jul 8, 2017

Choose a reason for hiding this comment

cartermp Jul 8, 2017

Choose a reason for hiding this comment

cartermp commented Jul 8, 2017

saul commented Jul 8, 2017

vasily-kirichenko commented Jul 8, 2017

cartermp commented Jul 8, 2017 • edited Loading

cartermp commented Jul 8, 2017

saul commented Jul 8, 2017

cartermp commented Jul 8, 2017

forki commented Jul 9, 2017

forki commented Jul 9, 2017

vasily-kirichenko commented Jul 9, 2017

forki commented Jul 9, 2017 • edited Loading

Krzysztof-Cieslak commented Jul 9, 2017

dsyme commented Jul 10, 2017

Pilchie commented Jul 10, 2017

Krzysztof-Cieslak commented Jul 11, 2017 • edited Loading

vasily-kirichenko commented Jul 12, 2017

Krzysztof-Cieslak commented Jul 12, 2017

vasily-kirichenko commented Jul 12, 2017

dsyme commented Jul 12, 2017

KevinRansom commented Jul 12, 2017

cartermp commented Jul 21, 2017

saul commented Jul 21, 2017

cartermp commented Jul 21, 2017

saul commented Aug 23, 2017

saul commented Aug 23, 2017

KevinRansom commented Aug 23, 2017

saul commented Aug 23, 2017

KevinRansom commented Aug 23, 2017

cartermp commented Aug 23, 2017

KevinRansom commented Aug 24, 2017

KevinRansom commented Aug 24, 2017

saul commented Aug 24, 2017

forki commented Aug 24, 2017 via email

realvictorprm commented Aug 24, 2017

saul commented Aug 24, 2017 via email

cartermp commented Aug 24, 2017

cartermp commented Aug 25, 2017

abelbraaksma commented Aug 25, 2017

saul commented Sep 2, 2017

saul commented Sep 2, 2017

saul commented Sep 4, 2017

KevinRansom commented Sep 6, 2017

saul commented Jul 7, 2017 •

edited

Loading

cartermp commented Jul 8, 2017 •

edited

Loading

forki commented Jul 9, 2017 •

edited

Loading

Krzysztof-Cieslak commented Jul 11, 2017 •

edited

Loading