-
Notifications
You must be signed in to change notification settings - Fork 5
Implement visitBooleanExpressionPredicate #91
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
Implement visitBooleanExpressionPredicate #91
Conversation
1e3cac6
to
4ec960e
Compare
@ExtendWith(MongoExtension.class) | ||
abstract class AbstractSelectionQueryIntegrationTests implements SessionFactoryScopeAware, ServiceRegistryScopeAware { | ||
|
||
SessionFactoryScope sessionFactoryScope; |
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.
SessionFactoryScope sessionFactoryScope; | |
protected SessionFactoryScope sessionFactoryScope; |
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.
protected
is more permissive than package-access, and making program elements more accessible than needed is something that we agreed not to do. Does this field and testCommandListener
have to be protected
?
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.
well, from visibility perspective, protected
is not mandatory; but it is also a good practice to use protected
on abstract class to denote it is accessible to any child class, regardless of whether the child class resides in the same package or not. it is more about semantic connotation.
For that reason, I want to follow @vbabanin's suggestions.
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 initially assumed this class might be reused across packages. However, if it’s meant to be used only within the same package and based on the earlier discussions - package-private
should be more appropriate.
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 switched to package-private
for the following two methods:
SessionFactoryScope getSessionFactoryScope() {
return sessionFactoryScope;
}
TestCommandListener getTestCommandListener() {
return testCommandListener;
}
...tionTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/com/mongodb/hibernate/query/select/Book.java
Outdated
Show resolved
Hide resolved
...Test/java/com/mongodb/hibernate/query/select/BooleanExpressionPredicateTranslatingTests.java
Outdated
Show resolved
Hide resolved
...Test/java/com/mongodb/hibernate/query/select/BooleanExpressionPredicateTranslatingTests.java
Outdated
Show resolved
Hide resolved
...Test/java/com/mongodb/hibernate/query/select/BooleanExpressionPredicateTranslatingTests.java
Outdated
Show resolved
Hide resolved
...tionTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
…stractSelectionQueryIntegrationTests.java Co-authored-by: Viacheslav Babanin <[email protected]>
…oleanExpressionPredicateTranslatingTests.java Co-authored-by: Viacheslav Babanin <[email protected]>
…nWhereClauseIntegrationTests
… protected access methods
...tionTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java
Show resolved
Hide resolved
...tionTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java
Show resolved
Hide resolved
…stractSelectionQueryIntegrationTests.java Co-authored-by: Viacheslav Babanin <[email protected]>
…stractSelectionQueryIntegrationTests.java Co-authored-by: Viacheslav Babanin <[email protected]>
…ss methods in AbstractSelectionQueryIntegrationTests
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.
LGTM!
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.
Skimmed through, did not review.
https://jira.mongodb.org/browse/HIBERNATE-78
A straightforward ticket