Visitor for UpdateClause class hierarchy #21
Conversation
| * @return the possibly transformed value resulting from the visitation. | ||
| */ | ||
| default <T> T accept(UpdateClauseVisitor<T> visitor) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
We had gotten advice that we shouldn't use defaults, simply leave as abstract.
|
|
||
| ExpressionPath getPath(); | ||
|
|
||
| class SetCommand implements UpdateCommand { |
There was a problem hiding this comment.
Can you use Immutables for these two?
There was a problem hiding this comment.
Done, but needed to move class higher so it would generate Immutable class.
There was a problem hiding this comment.
This seems incorrect: immutables/immutables#23 ?
Or is it the case that an immutable can only work if it's nested in a parent immutable?
There was a problem hiding this comment.
I have corrected this. I moved the interface UpdateCommand into the TestUpdateClauseVisitor class and nested the RemoveCommand and SetCommand classes under that. It all looks much better now. I think IntelliJ was not refreshed before I checked for the generated test classes in generated-test-sources.
| } | ||
|
|
||
| void testSetClause(UpdateClause clause, ExpressionPath path, Entity entity) { | ||
| final UpdateCommand command = clause.accept(VISITOR); |
There was a problem hiding this comment.
How come this isn't used for the RemoveClause as well?
There was a problem hiding this comment.
If you mean why isn't testSetClause() used for remove? Remove doesn't use an entity. Unless you are suggesting I use Optional instead?
There was a problem hiding this comment.
It's a minor nit, but it seems that you could remove the entity assertion and add it to the specific tests, or have another method call this one. This comment is minor, do what you think is best.
There was a problem hiding this comment.
I decided moving the assertion to the specific test was the cleanest.
Common PR for update expression for use by MongoDB and RocksDB implementations.