-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-3977: Refactor GroupForLinq3 to return an explicit type. #693
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
Conversation
/// </summary> | ||
public PipelineStageDefinition<IGrouping<TValue, TInput>, TOutput> ProjectStage { get; } | ||
|
||
internal GroupForLinq3Result(PipelineStageDefinition<TInput, IGrouping<TValue, TInput>> groupStage, PipelineStageDefinition<IGrouping<TValue, TInput>, TOutput> projectStage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructors before properties
/// <summary> | ||
/// The resulting group stage. | ||
/// </summary> | ||
public PipelineStageDefinition<TInput, IGrouping<TValue, TInput>> GroupStage { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line between properties when using XML doc comments.
ProjectStage = projectStage; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the following Deconstruct
methods so that we can deconstruct it just like we can a Tuple
:
/// <summary>
/// Deconstructs this class into its components.
/// </summary>
/// <param name="groupStage">The group stage.</param>
/// <param name="projectStage">The project stage.</param>
public void Deconstruct(
out PipelineStageDefinition<TInput, IGrouping<TValue, TInput>> groupStage,
out PipelineStageDefinition<IGrouping<TValue, TInput>, TOutput> projectStage)
{
groupStage = GroupStage;
projectStage = ProjectStage;
}
var (groupStage, projectStage) = PipelineStageDefinitionBuilder.GroupForLinq3(id, group); | ||
return aggregate.AppendStage(groupStage).AppendStage(projectStage); | ||
var stages = PipelineStageDefinitionBuilder.GroupForLinq3(id, group); | ||
return aggregate.AppendStage(stages.GroupStage).AppendStage(stages.ProjectStage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change.
var (groupStage, projectStage) = PipelineStageDefinitionBuilder.GroupForLinq3(id, group, translationOptions); | ||
return pipeline.AppendStage(groupStage).AppendStage(projectStage); | ||
var stages = PipelineStageDefinitionBuilder.GroupForLinq3(id, group, translationOptions); | ||
return pipeline.AppendStage(stages.GroupStage).AppendStage(stages.ProjectStage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change.
@@ -234,20 +234,20 @@ public void PipelineStageDefinitionBuilder_Group_with_expressions_should_throw_w | |||
[Fact] | |||
public void PipelineStageDefinitionBuilder_GroupForLinq3_with_expressions_should_throw_with_LINQ2() | |||
{ | |||
var (groupStageDefinition, projectStageDefinition) = PipelineStageDefinitionBuilder.GroupForLinq3((BsonDocument x) => 1, x => new { Count = x.Count() }); | |||
var stages = PipelineStageDefinitionBuilder.GroupForLinq3((BsonDocument x) => 1, x => new { Count = x.Count() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change and all others in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm open to a better name for the return type. I debated whether to make it a nested type inside
PipelineStageDefinitionBuilder
but decided against it. I'd be willing to reconsider that decision if you think it's a better choice.