Refactoring of MissingQuery#12030
Refactoring of MissingQuery#12030alexksikes wants to merge 0 commit intoelastic:feature/query-refactoringfrom
Conversation
There was a problem hiding this comment.
make this case 4 instead of default?
There was a problem hiding this comment.
the method needs to return though
There was a problem hiding this comment.
As this is only in the test I think we are save assuming that randomIntBetween can only return values between 0 and 3, so leaving the largest value as default should be ok. Current implementation is fine as well of course.
|
left two tiny comments, looks good though |
|
@javanna Thanks for the review. I updated the PR accordingly. |
There was a problem hiding this comment.
before you would randomly return unmapped field names, now you never do. randomIntBetween(0, 4) was ok, case 4 was ok too, you can just throw UnsupportedOperationException from the default or something.
|
left a couple of minor comments, @cbuescher do you mind having a look too? |
There was a problem hiding this comment.
Reading just the method name, I was curious what it means in this context. Maybe short comment?
There was a problem hiding this comment.
good point, actually given that it's a public method some javadocs wouldn't hurt
There was a problem hiding this comment.
yes I'll add these, thanks for seeing this.
|
Also did a round of reviews, left some remarks. |
|
@javanna @cbuescher thanks for the review. I updated the PR accordingly. |
There was a problem hiding this comment.
I would rather test this by calling the newFilter static method. In fact the check that we left there instead of relying on validate is there ony to protect other code paths from the outside (newFilter is public)
|
did another round, left a few small comments |
|
@javanna I updated the PR. Thanks. |
There was a problem hiding this comment.
Does this work? Above PROTOTYPE is initialized with null which will result in an exception being thrown here.
|
Left a few comments. |
|
@MaineC Thanks for the review. I updated the PR. |
d9398ce to
64a50fd
Compare
Relates to #10217
This PR is against the query-refactoring branch.