This change adds a new document covering some "collection best
practices" for the Razor repo. This can be expanded, but I think/hope
it's a good start.
Fixes https://github.com/dotnet/razor/issues/10842
The formatting self-nerd-sniping continues.
The formatting engine was written to use the LSP `TextEdit` class, which
makes some sense, but also uses Roslyn APIs like `SourceText` a lot,
which uses the `TextChange` struct instead. This meant lots of code to
convert to and from the two types. Changing the whole formatting engine
over to `TextChange`, and using more `TextSpan`, `LinePositionSpan` etc.
removes a lot of this code. It also makes a lot more sense in cohosting,
to boot.
I wouldn't claim that I've gone through and improved the perf of the
formatting engine, but rather I've use the changes to lead me to things
that need fixing. ie, I started out moving from `TextEdit[]` to
`ImmutableArray<TextChange>`, and this let me to places where pooled
array builders could be used, and places where `Range` and `Position`
were used which didn't make much sense, and then the constructor for
`LinePosition` threw at one point because it turns out we were only
using the `Line` property from the `Position` that used to be used, and
so never validated the characters, so that API moved to `int`, etc.
TL;DR the commits tell the story, and there could well be something I
missed, if it never came across my plate for another reason.
For a long while, the `GetLanguageKind(...)` method that determines
whether an index into a document falls within Razor, C# or HTML has been
a bit of a wart on the `IDocumentMappingService`. It really isn't part
of document mapping, and its implementation is completely distinct. In
fact, making the actual change is quite simple, so why hadn't it been
done yet? The answer is mocking.
There are several tests that mock
`IDocumentMappingService.GetLanguageKind(...)` to lie about test inputs.
In my not-so-humble opinion, this represents an abuse of mocking.
Instead of setting up tests to have the necessary inputs that ensure
`GetLanguageKind(...)` would return a real and correct result, the
inputs would often be garbage and an `IDocumentMappingService` mock
would lie about the `GetLanguageKind(...)` result at a particular
location. This makes moving `GetLanguageKind(...)` off of
`IDocumentMappingService` a much larger change than it needs to be. This
is why there are substantial test changes in this PR.
Don't misunderstand me as a mocking hater! Mocking libraries are
definitely useful! In fact, there are new mocks used in this very PR!
However, mocks should be used judiciously and thoughtfully, and in this
case, a mock was used to write lazy tests.
Fixes https://github.com/dotnet/razor/issues/8774
Most places already passed an ImmutableArray here. I left the method itself being an iterator, as 50% of callers need an ImmutableArray result, but the other 50% need an array, so no clear winner.
This commit wasn't purely mechanical, but it was close. Just making things mostly compile, no optimizations or anything yet, and probably still a bunch more renames to come.
* OnAutoInsert Cohosting Tests
* Fixing C# case (and correcting others)
All text should already be in the document/buffer when OnAutoInsert is being executed. Tigger character is not being added to the buffer, it should already be in the buffer.
* PR feedback
Switching to applying edit instead of verifying edit contents and range. Switching from Theories to separate Facts where input was complex. Other misc cleanup.
* Fixing options source and adding options tests
* Tests for all options
* Switching to use TestCode class
* Create options object for cohost OnAutoInsert to combined individual options passed to the remove service.
* More PR feedback
* Switching to nested RazorFormattingOptions
David Wengier
Dustin Campbell
dotnet-maestro[bot]
Alex Gavrilov