Reformat indentation on paste #4702
Conversation
saul
changed the title
| @@ -22,6 +23,10 @@ type internal FSharpEditorFormattingService | |||
| checkerProvider: FSharpCheckerProvider, | |||
| projectInfoManager: FSharpProjectOptionsManager | |||
| ) = | |||
|
|
|||
| static let toIList (xs : 'a seq) : IList<'a> = ResizeArray(xs) :> _ | |||
There was a problem hiding this comment.
Just a nit: do static let toIList (xs : 'a seq) = ResizeArray(xs) :> IList<'a>
|
This is cool! It will make the experience much better. :) |
|
|
||
| static let toIList (xs : 'a seq) : IList<'a> = ResizeArray(xs) :> _ | ||
|
|
||
| static let getIndentation : string -> int = Seq.takeWhile ((=) ' ') >> Seq.length |
There was a problem hiding this comment.
And this is just after Don's talk where he said "do not use point free, just don't" ;)
There was a problem hiding this comment.
I haven’t watched it yet... my apologies to Don!
| let nextLineShouldBeIndented = FSharpIndentationService.IndentShouldFollow(documentId, sourceText, filePath, span.Start, parsingOptions) | ||
|
|
||
| let removeIndentation = | ||
| let nextLineIndent = fixedPasteText.Lines.[1].ToString() |> getIndentation |
There was a problem hiding this comment.
Is .[1] safe here?
There was a problem hiding this comment.
There’s a check above that checks that the span is multi line
There was a problem hiding this comment.
It seems difficult to see that check. Can we explicitly guard right before it for sanity?
| if not (String.IsNullOrEmpty(previousLine.ToString())) then | ||
| Some previousLine | ||
| else | ||
| tryFindPreviousNonEmptyLine (l - 1) |
There was a problem hiding this comment.
let tryFindPreviousNonEmptyLine l =
seq { for i in [l - 1 .. -1 .. 0] -> sourceText.Lines.[i] }
|> Seq.tryFind (not << String.IsNullOrEmpty)A bit more safe variant :)
|
It does not work for me:
|
|
Do you have indentation mode set to 'Smart'? |
|
Yes. |
|
I've restarted VS, the same behaviour. |
|
The vsix seems to be installed:
|
|
|
|
Your code works on my machine 🤷♂️ |
|
But the code looks like this at one moment:
Is it ok?.. |
|
Sorry, maybe I don't understand how the feature should work... |
|
Well that's where my caret was when I hit paste. I did have to manually de-indent to that level... I was copying your GIF verbatim. To answer your question as to how it should work, code is re-indented so that it matches the indentation level of your caret when you paste. It would be easy to swap it around so that it used the correct indentation level but that's where the caret naturally ends up anyway (thanks for the indentation service). |
|
I'm lost :( So, everything is OK? |
|
This looks to be how it works in VS for Mac too. Indentation of the paste is adjusted to the caret column. Was one of the first things I worked on. |
|
@vasily-kirichenko maybe this GIF demonstrates what I'm trying to say better. Notice where the caret is when I hit paste - that's the only thing that matters:
|
|
I can't tell from the gif, but does it also work when the selection is irregular like this (the selection doesn't start from column 0, but subsequent lines do) ?
|
|
Indeed it does - see the GIF in the PR description. |
|
@saul Ah, I see. Thanks :) |
|
Tet failure is in Async and seems unrelated to this PR. @dsyme? |
|
@KevinRansom is still on point to clean out the bit rot in some legacy tests. Until then, no updates. |
|
But a load of otherwise functioning tests have been disabled? |
|
I find it absolutely unacceptable to happily merge PRs having a lot of tests disabled. /cc @dsyme |
|
This PR is also blocked on the fact that I want to write unit tests for new functionality but can't actually test it 😢 |
|
@saul it's even more strange that some tests from Roslyn folder are passed locally (e.g. ColorizationServiceTests), but are not enabled on CI. |
I'll have to defer to @KevinRansom on the best way to proceed w.r.t tests, as this isn't my call. Most tests should be just fine, and I believe the Roslyn-based ones have no issues with bit-rot at all, since they're the ones which have been actively developed. |
|
@saul -- I'm sorry you are blocked. I had to switch away to 4.5 for a short while. |
|
@dotnet-bot test this please |
|
@saul now that IDE tests are back up and running in CI we can take this (assuming approval) |
|
I’m on vacation for 10 days - I’ll take a look at this when I get back. I really want this in! |
|
Cheers, and happy vacation!
On Sat, Jul 21, 2018, at 13:53, Saul Rennison wrote:
I’m on vacation for 10 days - I’ll take a look at this when I get
back. I really want this in!> — You are receiving this because you were mentioned. Reply to this
email directly, view it on GitHub[1], or mute the thread[2].
|
|
@dotnet-bot test this please |
|
@saul We'd love to take this for VS 2019 Previews. Do you want to resolve conflicts? |
|
This is pretty much finished - I’ll try running (and fixing) the tests when I’m back from NY. Edit: Just realised that was my excuse 2 months ago... maybe I should stop going on vacation! |
|
You should never stop going on vacation (especially if you ever end up working in the US, where vacation is a rarity)! No pressure at all. |
|
Tests are green, this is ready to merge @cartermp. BTW - haven't really done any work in this repo for a few months, but the Azure DevOps is way nicer than what was there before. Also being able to run unit tests in the IDE (instead of them being EXEs) made debugging this significantly easier. A lot of contributor pain has gone since I last worked on this repo! Great work guys. |
There was a problem hiding this comment.
Looks like some FSComp changes are getting pulled in when they shouldn't be. Can you undo these?
Separately, @brettfo @KevinRansom this isn't the first time a local change to these files got pulled in by a pull request. Is there something funky going on with these files?
|
probably needs rebasing, I'll take care of it. |
|
@KevinRansom This now implicitly reverted multiple compiler message changes that we've taken over time. Looking at things a bit more deeply, we've merged in multiple such changes that have caused some compiler messages to be the new ones, and some being reverted back to older ones. This PR actually included a change that brought back an improvement while regressing in others. Can we please revert this PR until we take care of that file? |
|
I will go through the resources files and fix them up. |
|
Spoke with @KevinRansom and these files are build outputs that are an unfortunate consequence of our build today. This issue shouldn't persist once the .NET SDK work is completed. |
* Format indentation on paste * Fix pasting when next line should be indented * Initial pass at 'Formatting' page * Add some tests * Fix tests
Notice how in the GIF indentation is automatically restructured depending on where you paste. The lack of this feature has been bothering me since day 1 as I cut/paste code around into different scopes often. Near enough feature complete already.
Todo: