Fixes Grammar in SourceTextContainer exception message#203
Fixes Grammar in SourceTextContainer exception message#203Pilchie merged 5 commits intodotnet:masterfrom Giftednewt:master
Conversation
…sts for SourceContainerExtensions
There was a problem hiding this comment.
Do you know what happened to this?
There was a problem hiding this comment.
Not sure how that got removed. I didn't actually add or remove anything from the project file so I'll revert the change.
…ng newline back onto EditorServicesTest.csproj
There was a problem hiding this comment.
Isn't this just testing the behavior of the mocking framework?
There was a problem hiding this comment.
The intention was to test that the GetTextBuffer extension method threw an argument exception when provided with a SourceTextContainer that was not a TextBufferContainer or derived type. Would it have been better to have written it as
Assert.Throws<ArgumentException>(() => Microsoft.CodeAnalysis.Text.Extensions.GetTextBuffer(containerMock.Object));
Or something similar?
I did also look at asserting on the ArgumentException message to make sure that the update resource text was returned however as this also returns the parameter name as part of the message I wasn't confident that this wouldn't make the test too brittle because someone could easily break the test just by changing the parameter name.
There was a problem hiding this comment.
Ahh, both @rchande and I were tricked by the extension method, so switching to the more obvious invocation would probably prevent others from being confused as well.
|
@Giftednewt did you want to go ahead and make those changes? |
|
@Pilchie apologies, been working round the clock for my day job. I'm making the changes now. |
|
Thanks @Giftednewt. Can you go ahead and sign the CLA at https://cla2.dotnetfoundation.org/? |
|
@Giftednewt, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
There was a problem hiding this comment.
Can you switch this to use nameof while you're already in there?
|
Thanks! |
Fixes Grammar in SourceTextContainer exception message
davkean
added
the
Community
7 participants
This resolves #38 by fixing the grammar, removing the unused resource value and adding unit tests for SourceTextContainerExtensions