Also delete all of the if-def-ed out and redundant code as a result. If we need to do any caching of things, lets just use xunit features and not odd static stuff with custom test attributes etc.
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.
dotnet-maestro[bot]
Jan Jones
David Wengier
Dustin Campbell
Alex Gavrilov