CSHARP-4894: Mark API that will be removed in 3.0 as obsolete: MongoDB.Driver.Core #1251
Conversation
There was a problem hiding this comment.
Overall I see no serious issues with using record for our settings classes (and other classes also).
There is still an issue when calling a constructor with how to tell the difference between the caller omitting an optional argument and passing null. In the first case we might want to supply a default value, in the second we would want to respect the explicitly passed null.
new Settings() ; // assigns a default value to `OptionalProperty`
new Settings(optionalProperty: null); // also assigns a default value to `OptionalProperty` instead of null
It might be that the only way a user can explicitly set a property to null is:
new Settings() with { OptionalProperty = null };
| public sealed record ConnectionSettingsNew | ||
| { | ||
| private readonly string _applicationName; | ||
| private readonly IReadOnlyList<IAuthenticatorFactory> _authenticatorFactories; // Should we use ImmutableArray instead ? |
There was a problem hiding this comment.
ImmutableArray seems to only be available on limited target platforms.
| bool loadBalanced = false, | ||
| TimeSpan? maxIdleTime = default, | ||
| TimeSpan? maxLifeTime = default, | ||
| string applicationName = default) |
There was a problem hiding this comment.
Should we consider removing all optional parameters from the constructor? Optional parameters can be set using with.
| /// <param name="applicationName">The application name.</param> | ||
| public ConnectionSettingsNew( | ||
| IEnumerable<IAuthenticatorFactory> authenticatorFactories = default, | ||
| IEnumerable<CompressorConfiguration> compressors = default, |
There was a problem hiding this comment.
It might not be relevant to this property, but what if the user wants to set the compressors to null?
The constructor can't tell the difference between the caller omitting the optional argument or passing null.
| { | ||
| internal static class CollectionsExtensions | ||
| { | ||
| public static IReadOnlyList<T> AsReadOnlyNonEmptyList<T>(this IEnumerable<T> enumerable) => |
There was a problem hiding this comment.
I would name this ToReadOnlyNonEmptyList<T> because more likely than not it's a conversion and not a cast.
| @@ -176,7 +177,7 @@ public static T IsEqualTo<T>(T value, T comparand, string paramName) | |||
| /// <param name="value">The value of the parameter.</param> | |||
| /// <param name="paramName">The name of the parameter.</param> | |||
| /// <returns>The value of the parameter.</returns> | |||
| public static TimeSpan IsGreaterThanZero(TimeSpan value, string paramName) => | |||
| public static TimeSpan IsGreaterThanZero(TimeSpan value, [CallerArgumentExpression("value")]string paramName = "value") => | |||
There was a problem hiding this comment.
Once you use CallerArgumentExpression it's no longer a paramName, it's a string representing the expression the caller used to provide the argument when calling this method.
There was a problem hiding this comment.
In the next line IsGreaterThan expects a parameter name as its last argument, not a string representing the caller's argument expression.
There was a problem hiding this comment.
The net result is that the following line of code on line 97:
throw new ArgumentOutOfRangeException(paramName, message)
might be called with a value for paramName that is not a parameter name, but rather a string representing an Expression. That seems wrong.
src/MongoDB.Driver/RenderContext.cs
Outdated
| /// Encapsulates classes needed for rendering Builder definitions. | ||
| /// </summary> | ||
| /// <typeparam name="TDocument">The type of the document.</typeparam> | ||
| public sealed record RenderContext<TDocument> |
There was a problem hiding this comment.
The old class name was SearchDefinitionRenderContext.
Are you implying that this RenderContext class will be used in more places?
There was a problem hiding this comment.
Seems like you are proposing replacing multiple Render arguments with an instance of this class.
In which case the name RenderArgs might be more appropiate.
There was a problem hiding this comment.
And should probably be a a struct to avoid an extra memory allocation.
src/MongoDB.Driver/RenderContext.cs
Outdated
| public IBsonSerializer<TDocument> DocumentSerializer | ||
| { | ||
| get => _documentSerializer; | ||
| init => _documentSerializer = Ensure.IsNotNull(value, nameof(value)); |
There was a problem hiding this comment.
A good example of why I prefer we standardize on using explicit backing fields (like _documentSerializer) instead of auto properties.
BorisDog
changed the title
No description provided.