CSHARP-5481: ScalarDiscriminatorConvention class should implement IScalarDiscriminatorConvention interface. #1607
Conversation
papafe
left a comment
papafe
left a comment
There was a problem hiding this comment.
Looks super good, and it's incredible how much easier is this compared to the other PR, good job!
I'm not LGTM at the moment mostly because OfTypeMethodToAggregationExpressionTranslator is a little bit over my head, so I'm waiting for someone else to take a look. And also waiting for the CI to be fixed 😄
|
This PR has a bit more in it than just the changes to I ended up with more changes than that because:
|
| if (actualType == type || actualType.IsSubclassOf(type)) | ||
| { | ||
| discriminators.Add(discriminator); | ||
| } | ||
| } |
There was a problem hiding this comment.
We could short circuit out of the loop after finding the actual type or a subtype as we shouldn't have two different subtypes with the same discriminator. Not sure if it's really worth it, maybe others have thoughts on this.
There was a problem hiding this comment.
Good point that we are only expecting one match, so we could stop after finding it.
But I think even better would be to verify that there is only one match, replacing lines 791-797 with:
var matchingType = actualTypes.SingleOrDefault(t => t == type || t.IsSubclassOf(type));
if (matchingType != null)
{
discriminators.Add(discriminator);
}
tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5481Tests.cs
Show resolved
Hide resolved
tests/MongoDB.Bson.Tests/Serialization/Serializers/DiscriminatorTests.cs
Show resolved
Hide resolved
| protected void AssertStages(IEnumerable<BsonDocument> stages, IEnumerable<string> expectedStages) | ||
| { | ||
| stages.Should().Equal(expectedStages.Select(json => BsonDocument.Parse(json))); | ||
| stages.Should().Equal(expectedStages.Where(x => x != null).Select(json => BsonDocument.Parse(json))); |
There was a problem hiding this comment.
Should we sync the changes to newly created LinqIntegrationTest class for easier migration of the tests in future?
There was a problem hiding this comment.
Yes. Now that it is merge to master.
I will do that.
src/MongoDB.Bson/Serialization/Conventions/ScalarDiscriminatorConvention.cs
Show resolved
Hide resolved
BorisDog
left a comment
BorisDog
left a comment
There was a problem hiding this comment.
I like this much simpler alternative, great PR!
Just a few styling suggestions.
...tionExpressionTranslators/MethodTranslators/OfTypeMethodToAggregationExpressionTranslator.cs
Show resolved
Hide resolved
...tionExpressionTranslators/MethodTranslators/OfTypeMethodToAggregationExpressionTranslator.cs
Show resolved
Hide resolved
rstam
force-pushed
the
5481
branch
2 times, most recently
from
cfb1c33 to
b26668f
Compare
…e correct location.
…alarDiscriminatorConvention interface.
5 participants
No description provided.