-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[JAVA-2635] Add restrictSearchWithMatch to GraphLookupOptions #422
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
Can you help me to see what went wrong: Execution failed for task ':driver-core:codenarcTest'.
I don't know how to access this file. Because, it work on my machine |
I checked out this branch and ran ./gradlew driver-core:codenarcTest and I see the failures. It's trailing whitespace on 7 lines of AggregatesFunctionalSpecification.groovy. The line numbers are 392, 416, 420, 443, 466, 467, 470 |
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.
This looks great. Just a few comments/questions.
* | ||
* @param filter the filter expression | ||
* @return this | ||
*/ |
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
@since 3.6
|
||
/** | ||
* @return the filter expression | ||
*/ |
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
@since 3.6
@@ -26,6 +28,7 @@ | |||
public final class GraphLookupOptions { | |||
private Integer maxDepth; | |||
private String depthField; | |||
private Bson restrictSearchWithMatch; |
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.
Remove extra space before field name.
fromHelper?.drop() | ||
} | ||
|
||
@IgnoreIf({ !serverVersionAtLeast(3, 4) }) |
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.
This test seems redundant. Is this testing anything that's not already being tested above?
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.
Yes, it's redundant. I have added 2 tests :
- with only restricSearchWithMatch option :
new GraphLookupOptions().restrictSearchWithMatch(eq('hobbies', 'golf')))
- with all options ;
new GraphLookupOptions().depthField('depth').maxDepth(0).restrictSearchWithMatch(eq('hobbies', 'golf')))
I can remove one test. Which ?
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.
I'd remove the second one with all options
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.
done !
I want to get this into 3.6 so can you revert the last commit? Thanks! |
This reverts commit 644a5a9.
Done ! |
Thank you for the contribution. Regards, |
No description provided.