CSHARP-5625: Support text indexes for explicit encryption #1770
Conversation
| /// </remarks> | ||
| public sealed class PrefixOptions | ||
| { | ||
| private readonly int _strMaxQueryLength; |
There was a problem hiding this comment.
I tried to follow the pattern already present in this file. I didn't want to mix auto props pattern with the existing backing field pattern. As a matter of fact, I don't think we have a general guideline in the driver, do we generally prefer auto props pattern vs backing field pattern?
There was a problem hiding this comment.
We don't have a general guideline, and we do have mixed styles. I do prefer auto props and recommend for others as well.
I don't see an issue with mixing styles within the same file. Also EncryptOptions can be refactored to use autoprops.
There was a problem hiding this comment.
I strongly dislike auto-properties for all but the very simplest data only classes, and maybe not even then.
My issue is that when reading a class it is helpful to know what state the class maintains. When you have explicit fields at the beginning of the file you can easily tell what state the class maintains.
Auto-properties are harder to find, and so the state the class maintains is spread out a bit more.
The worst-case scenario is a class that has some explicit fields and some auto-properties, because then the state the class maintains is really hard to discern. For that reason, we should never mix the two.
To me auto properties are just lazy syntactic sugar that usually just make code harder to understand.
There was a problem hiding this comment.
I'll leave the changes as is just because I don't like mixing patterns.
|
|
||
| RunTestCase(clientEncryption, prefixSuffixCollection, substringCollection); | ||
| } | ||
| return; |
There was a problem hiding this comment.
minor: no need for return?
There was a problem hiding this comment.
I was following the rider suggestion to include an explicit return to enhance the clarity of the method control flow for readers which is fair given that the method has a multiple local functions. Do we generally not care about this?
There was a problem hiding this comment.
I don't think we've notices that in other places. From a first glance having empty returns feels awkward.
Maybe not in this case, but in general if the method in not very readable, we probably should just refactor it.
@sanych-sun wdyt about empty returns?
...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs
Outdated
Show resolved
Hide resolved
...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs
Outdated
Show resolved
Hide resolved
| "$expr", new BsonDocument | ||
| { | ||
| { | ||
| $"{operation}", new BsonDocument |
There was a problem hiding this comment.
@rishitb-mongodb we probably will need builders for $strEndsWith, $strStartsWith, $strContains.
But only when it will be out of preview. WDYT?
adelinowona
force-pushed
the
csharp5625
branch
from
f421b90 to
469a2f5
Compare
Added TextPreview support for Prefix/Suffix/Substring Indexes