Accommodate changes to XContent classes #125
Conversation
There was a commit to OpenSearch core to move XContent classes, forcing us to update a bunch of imports. This change is effectively a cherry-pick of 87dafb0 without modifying the build.gradle file. Signed-off-by: Michael Froh <[email protected]>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## 2.x #125 +/- ##
============================================
+ Coverage 70.61% 77.44% +6.83%
- Complexity 197 206 +9
============================================
Files 29 29
Lines 878 878
Branches 120 120
============================================
+ Hits 620 680 +60
+ Misses 208 138 -70
- Partials 50 60 +10
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -76,7 +77,7 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| this.properties.writeTo(out); | |||
| } | |||
|
|
|||
| public static ResultTransformerConfiguration parse(XContentParser parser) throws IOException { | |||
| public static KendraIntelligentRankingConfiguration parse(XContentParser parser) throws IOException { | |||
There was a problem hiding this comment.
re:naming - is this going from less specific naming to more specific intentionally?
There was a problem hiding this comment.
Yes.
a) It helps the unit tests, since we can confirm the specific type.
b) There's a best-practice to take the most general type of argument and return the most specific type. I think it's an application of the Liskov substitution principle.
| @@ -154,7 +155,7 @@ public KendraIntelligentRankingProperties(final List<String> bodyFields, | |||
|
|
|||
| public KendraIntelligentRankingProperties(StreamInput input) throws IOException { | |||
| this.bodyFields = input.readStringList(); | |||
| this.bodyFields = input.readStringList(); | |||
| this.titleFields = input.readStringList(); | |||
There was a problem hiding this comment.
is this related to the XContent classes change?
There was a problem hiding this comment.
Not directly.
When merging the cherry-picked commit, I also picked up the unit tests that I had previously added (in KendraIntelligentRankingConfigurationTests).
When I added those tests in main, they also revealed some bugs (like this one). I should go through and make sure that all of the tests I added in main get backported to 2.x, since I fixed a bunch of little bugs along the way.
There was a problem hiding this comment.
Oh... I just added the backport 2.x label to #99.
There was a problem hiding this comment.
Oh, but the backport PR fails to build because we need to merge this PR to fix the XContent stuff:
There was a problem hiding this comment.
Oh, but the backport PR fails to build because we need to merge this PR to fix the XContent stuff:
Looks like there are two ways to handle this issue:
- In this PR, remove the content of Improve test coverage #99, merge this one and merge [Backport 2.x] Improve test coverage #126
- Change the description to include the backport of Improve test coverage #99, then merge this one; the backport PR [Backport 2.x] Improve test coverage #126 is pointless in this case because the code is already include.
I'm fine with either, but 1. appears to overreact, and the order of commits in 2.x is messed up comparing to main branch
There was a problem hiding this comment.
I think #126 shouldn't have any conflicts once this one is merged, because any overlapping changes should be identical.
There was a commit to OpenSearch core to move XContent classes, forcing us to update a bunch of imports.
This change is effectively a cherry-pick of
87dafb0 without modifying the build.gradle file.
Issues Resolved
#120
Check List
New functionality includes testing.New functionality has been documented.New functionality has javadoc addedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.