CSHARP-5966 Add support of string.TrimStart and TrimEnd methods to SerializerFinder#1953
CSHARP-5966 Add support of string.TrimStart and TrimEnd methods to SerializerFinder#1953kyra-rk wants to merge 6 commits intomongodb:mainfrom
Conversation
|
(will keep open until next week, once 3.8 is released) |
There was a problem hiding this comment.
Pull request overview
Adds LINQ3 serializer-finder support for string.TrimStart/TrimEnd so these method calls can be translated with the correct string serializer, and validates behavior with unit + integration tests.
Changes:
- Extend
SerializerFinderVisitormethod-call handling to recognizeTrimStartandTrimEnd. - Expand serializer-finder string method coverage to include
TrimStart(char[])andTrimEnd(char[]). - Add a Jira integration test validating translation to
$ltrim/$rtrimand expected results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/SerializerFinders/StringTests.cs | Adds serializer-finder coverage for TrimStart/TrimEnd returning StringSerializer. |
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5966Tests.cs | New integration tests validating $ltrim/$rtrim translation and runtime results. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs | Routes TrimStart/TrimEnd method calls through existing trim serializer deduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public class CSharp5966Tests : LinqIntegrationTest<CSharp5966Tests.ClassFixture> | ||
| { | ||
| public CSharp5966Tests(ClassFixture fixture) |
There was a problem hiding this comment.
I know it's technically outside of the ticket scope, but if we are adding integration tests for TrimStart and TrimEnd methods, let's use the "newly suggested approach" instead of Jira-Ticket-number class name. We can discuss the pros and cons of both approaches tomorrow after stand up.
There was a problem hiding this comment.
Sounds good. I'm not sure what you mean by newer approach, so will wait to do this after we chat
| [TestHelpers.MakeLambda((MyModel model) => model.Name.ToUpperInvariant()), typeof(StringSerializer)], | ||
| [TestHelpers.MakeLambda((MyModel model) => model.Name.Trim()), typeof(StringSerializer)], | ||
| [TestHelpers.MakeLambda((MyModel model) => model.Name.Trim(new char[] { ' ' })), typeof(StringSerializer)], | ||
| [TestHelpers.MakeLambda((MyModel model) => model.Name.TrimStart(new char[] { ' ' })), typeof(StringSerializer)], |
There was a problem hiding this comment.
Let's also add parameter-less methods as non-supported, probably together with the link to Jira ticket where we will implement them. Example of negative tests could be find here.
There was a problem hiding this comment.
How does the team usually link the Jira ticket? In Server it would have been something like // TODO CSHARP-5979 Make these tests supported, but the closest example I found was:
// https://jira.mongodb.org/browse/CSHARP-2678
if (IsRetryableWriteExceptionAndDeploymentDoesNotSupportRetryableWrites(exception))
{
....
}
There was a problem hiding this comment.
Also, iirc they had a PR-level check for any remaining TODO SERVER-XXXX tickets matching the ticket name. Not sure if the team has something like this already, or if might be worth trying
There was a problem hiding this comment.
Usually I put comment something like:
Not supported yet, see https://jira.mongodb.org/browse/CSHARP-2678
sanych-sun
left a comment
sanych-sun
left a comment
There was a problem hiding this comment.
Almost there. 1 comment + please investigate the failed unit test on net472. It's interesting case :). Reach me out if you need some clues and directions.
|
|
||
| public class TrimTests : LinqIntegrationTest<TrimTests.ClassFixture> | ||
| { | ||
| public TrimTests(ClassFixture fixture) |
There was a problem hiding this comment.
Please rename to StringTrimTests.
Ahhh, seems like |
No description provided.