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

Remove IDocumentMappingService.GetLanguageKind(...) and make it an extension method on RazorCodeDocument #10851

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Sep 6, 2024

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 #8774

@DustinCampbell DustinCampbell requested a review from a team as a code owner September 6, 2024 22:35
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.

Move GetLanguageKind method to somewhere else
2 participants