The reason why I created a separate AggregateFluentBuilder class with extension methods rather than modifying AggregateFluent was that it was necessary in order to have Atlas Search in a separate assembly. Now that we're integrating it into the driver, we might want to reconsider this decision.

Good point, removed.

Should the namespace here be MongoDB.Driver.Search, given that it's in a subdirectory?

Yes this breaks the convention. The intention was to make Search easily discoverable in intelligence, but still organize all multiple search file in same place. Now that we got IAggregateFluent.Search we can fix the namespace.

The reason why I introduced a new SearchBuilders class rather than extending the existing Builders class was that it isn't possible to add static extension methods to a class. This seems like another decision that would be worth reconsidering now that we're no longer forced into it. I imagine that it would be a better experience for customers not to have to keep track of type different classes.

Yes good point, I think it needs to be merged with Builders.

Agreed. This should be part of Builders<T> rather than a separate class.

We're restricting it to just Text for now and not adding support for any of the other operators?

Text restriction is just for this POC, the intention is to support all operators.

Agreed. This should be part of Builders<T> rather than a separate class.

This refactoring changes the error message a tiny bit. Could be considered a breaking change.

Same might apply to some of the other refactorings.

Do we really want one Ensure method calling another? Or should each Ensure method be complete by itself?

I think code reuse makes things simpler and cleaner, where appropriate.

Of course! But it changes the error messages.

I think it's a very minor change in this case.
We also need to change messages in IsNullOr* anyway at some point.

Normally should call WithPipeline and _pipeline.Search

... but I realize _pipeline.Search is not implemented yet.

Good point, added PipelineDefinitionBuilder.Search

I disagree with removing private backing fields.

I think this is a classic usage for auto properties.
The only purpose of this class is to expose those properties, so we can let it do just that.

My counter argument is that most of our classes already have private fields using the _field or __staticField naming convention.

In many cases (such as when validating in setters) an explicit backing field is required.

Why not just standardize on explicit backing fields and that way we can always know by glancing at the top of a class what state the class maintains.

Otherwise we have to tediously search the source file to see if there are any autoproperties we didn't know about.

I think the key here is finding a good balance between following the existing style and evolving our code base to use new language features and mainstream standards. We have already adopted some mainstream conventions like string interpolation, nameof, local methods and other, despite having most of the code written differently. I hope we continue to do so.
Autoproperties are a very good example of a "mainstream" and very loved feature by C# community (which continues to evolve), and I believe it is perceived as sort of "default" in C# dev process.

Like with fields, all properties (auto implemented or not) through the source file, they will always reside in same section, like fields. Given that, and the fact the major part of code is cleaned up, the "state" of the class appears more clear to me. Autoproperties indicate that this is just a simple data holders, no custom properties, no special relation between property and field that needs to be inspected.

For more complicated classes, where inspecting class state/functionality becomes is more complicated, and I find using VS built-in class/document explorers very useful.

Lots of irrelevant refactoring in this file.

Good opportunity for minor refactoring. For example if staying with fields, readonly needs to be added anyway.

Should the parameters be named highlightOptions and countOptions?

The JSON fields in $search are called highlight and count, but I have no objection to this change if you feel that it will make it easier for customers to understand what the parameters do.

I realized that the server uses the shorter names.

It's a conflict between our naming conventions and the server's.

I made this comment because normally a parameter names count would be of type int.

This variable is not not just the query... I suggest renaming it to something like renderedArgs.

Renamed query to searchDefinition, and renderedQuery to renderedSearchDefinition. @rstam do you think this is a better naming?

Not sure this builder is needed.

As you well know I don't like expression bodied methods, and this code is a prime example of why. The bad formatting makes the bracketed lines "look" like a body but its NOT!

I would write this like this:

public HighlightOptions<TDocument> Options(
 PathDefinition<TDocument> path,
 int? maxCharsToExamine = null,
 int? maxNumPassages = null)
{
 return new()
 {
 Path = path,
 MaxCharsToExamine = maxCharsToExamine,
 MaxNumPassages = maxNumPassages
 };
}

or if you insist on using an expression body for something that is not a one-liner at least format it so that it is more apparent that the bracketed code is NOT a body:

public HighlightOptions<TDocument> Options(
 PathDefinition<TDocument> path,
 int? maxCharsToExamine = null,
 int? maxNumPassages = null)
 => new()
 {
 Path = path,
 MaxCharsToExamine = maxCharsToExamine,
 MaxNumPassages = maxNumPassages
 };

Refactored all to use the last styling suggestion.

I know there's talk about moving all these classes up one level in the namespace, but maybe we should leave them here.

Being in their own namespace could help in avoiding future name collisions.

All search code was moved under MongoDB.Driver.Search, and the intention is to keep it that way.

The error message will be wrong when value is not null but not between.

The error message won't mention that null is valid.

This is probably a reason for each Ensure method to be self contained.

That's a good catch.
I still think that we should and can easily reuse the code.
We got similar behaviour in the old IsNull* methods. I suggest to fix them to in a separate ticket.

I realized that the server uses the shorter names.

It's a conflict between our naming conventions and the server's.

I made this comment because normally a parameter names count would be of type int.

My counter argument is that most of our classes already have private fields using the _field or __staticField naming convention.

In many cases (such as when validating in setters) an explicit backing field is required.

Why not just standardize on explicit backing fields and that way we can always know by glancing at the top of a class what state the class maintains.

Otherwise we have to tediously search the source file to see if there are any autoproperties we didn't know about.

Of course! But it changes the error messages.

This is an example of a class where we have to use explicit backing fields.

I would like us to standardize on this approach even in cases where autoproperties could be used.

Yes there always will be cases where non trivial setter is required.
That should not deter us from using autoproperties for simpler classes. And especially for cases where set is private or init only.

You don't think => should be indented one more tab stop?

Better yet use a method body... :)

You don't think { should be indented on tab stop further than => new()?

Personally I think => should stay in the line above. We can just evolve the standard we are using already (which is different than my original code here):
We already use:
T Method() => new T();
or

T Method() =>
 new T(1, 2, 3,....); { a = 1 }

for longer lines.
Evolving this further:

T Method() =>
 new T(1, 2, 3,....)
 {
 a = 1,
 b = 2,
 c = 3
 };

and in our case

T Method() =>
 new()
 {
 a = 1,
 b = 2,
 c = 3
 }

With this option it's clear that this is object creation and not a body.