Good catch, we don't, can be removed.

+1

Probably to exercise options == null path.

In that case that can be done elegantly, we should do that in a separate ticket for other methods as well. Usually we can address such improvements on green-build Fridays.

Alphabetical order.

Should we use SortDefinition<TDocument>?

Should we use SortDefinition<TDocument>?

Use SortDefinition<TDocument>.

Use SortDefinition<TDocument>.

Use SortDefinition<TDocument>.

Unfortunately UpdateOptions and ReplaceOptions are not generic classes, so we can't parametrize the properties. We can add the parameter, but I think it would be a breaking change.
We could also add the generic UpdateOptions<TDocument> alongside the non generic version and make new methods that use this, while deprecating the methods that are using the non generico version. This would be the long road

I have done this only for ReplaceOptions for now. If all I've done makes sense, I'll do the same for UpdateOptions.
This required to create a new class ReplaceOptions<T> and an implicit conversion operator from ReplaceOptions to ReplaceOptions<T>.

Is it possible to make WriteSort and pass model.Sort there? And move Render there as well.

It will be a code breaking change (client's code has to be rewritten) as far as I understood. Should we create a new overload instead? And support both ReplaceOptions and ReplaceOptions<TDocument>

@BorisDog WDYT?

Yup we avoid breaking changes. We'll probably need an extension method.

This looks like a binary breaking change but not a source code breaking change.

There is also an implicit conversion operator from ReplaceOptions to ReplaceOptions<TDocument> that provides backward compatibility.

Yes, I discussed this with Robert and this was an attempt to avoid creating more methods/overloads and keep source compatibility.
I was reading a little bit around online about semver in regards to binary compatibility. It seems that there is not a complete agreement on it, but overall it's recommended to increase major version with a binary-incompatible change.
Personally (irregardless of this case), I think this is a matter of deciding where do we stand on this and then take it from there.

I would prefer a solution that doesn't involve doubling (or worse) the number of overloads. Too many overloads is a bad thing.

The current approach uses an implicit conversion from ReplaceOptions to ReplaceOptions<TDocument> to preserve backward compatibility.

Another approach could be to use subclassing: ReplaceOptions<TDocument> : ReplaceOptions. Then we could leave the current overloads unchanged at the expense of some type safety. The code that uses the replaceOptions would have to test whether the passed in options are the subclass or not.

Given what we discussed at Triage today I'll park this ticket until next when we can have a more general discussion about breaking changes in general

I have to apologize, I did not notice the implicit conversion. So this is not code breaking change.
On other hand I suppose it's better to follow Robert's suggestion with using of subclasses as clearer way to do the same thing. At least because this way we could avoid changes to public API.

I went for Robert suggestion and made the generic options derive from the base class we have now

Need to ensure that this change is not breaking.

Yup we avoid breaking changes. We'll probably need an extension method.

This is not binary compatible.
The alternative would be to add other two constructors (given that we already have two) but:

• Then we have 4 very similar constructors
• Developers using those constructors will have errors due to constructor ambiguity if they do not specify all parameters. This is theoretically still source compatible, but definitely annoying
  Any suggestions about this? Personally I'd just keep the sort definition out of the constructor if we cannot find another alternative

What if we add new constructor with sort as the very last non-optional parameter? In such way it would not be a breaking change and there would not be an ambiguity.

That's a good idea, I didn't think about it :)
I've added two new constructor to mirror the ones we have already, and added a new private one that is being called by all the others

Nice! And I suppose the plan should be to eliminate this copied ctors in next major release, by making the sort parameter as optional. So the question is: how we going to track this and not forget to do?

Yup, that would be the plan! I've added the constructors and the non generic options to the list of breaking changes for next version.

Should we revert this change as we should be backward compatible now?

Definitely

What if we add new constructor with sort as the very last non-optional parameter? In such way it would not be a breaking change and there would not be an ambiguity.