Repository navigation
Conversation
- Add SearchImage (opaque id + passageId) and SearchResults.images - Accept "images" in unified search fields - Default images to empty when the platform omits them - Document image results in README and CONTEXT.md
✅ Commit Lint: passedAll commit messages in this PR conform to Conventional Commits. 📦 Release preview:
|
- Restore the 5-argument constructor and copy as hidden overloads - Mark them for removal in 3.0.0
- Replace the hidden 5-argument constructor with @jvmoverloads
camrun91
left a comment
There was a problem hiding this comment.
nice work, tests and api dump look solid. requesting changes for a few things, details inline. the big ones: one bad image in the response shouldn't take down all of unified search, and the "source-compatible, no major bump" claim doesn't hold for java callers. also, can you retitle this to feat(core): return images from unified search to match the commit? otherwise a squash merge gets no minor bump or changelog entry. and squash the two fix(core) commits into the feat commit so they don't show up in the release notes as bug fixes for something that never shipped.
- Move SearchResults.images into the body with a new constructor so 2.6.0's constructor, copy, componentN and equals are unchanged - Make SearchImage a plain class instead of a data class - Drop a malformed image instead of failing the whole search - Clarify that the KDoc drop rules apply to verse and image results - Reword the README search bullet so images read as part of unified search
Code Coverage ReportOverall Line Coverage: 92.4%
Generated from Kover XML report. |
camrun91
left a comment
There was a problem hiding this comment.
thanks for the quick turn, the compat rework fixes the java break and the plain SearchImage is good. still a few things before this can go in:
imagesisn't part ofequals/hashCode/toStringandcopydrops it. that'll bite anyone holding results in state. see inline."images": nullstill fails the whole search.- please retitle to
feat(core): return images from unified search. squash merge uses the title as the commit subject, so right now this won't version or show up in the changelog as a feature. squashing thefix(core)commits would be nice too.
ci's green and tests look good otherwise.
| * The matching images, in rank order. Empty when the platform does not offer image results. | ||
| * Not carried over by [copy] and not compared by [equals]. | ||
| */ | ||
| var images: List<SearchImage> = emptyList() |
There was a problem hiding this comment.
since images is a body property, equals/hashCode/toString ignore it and copy(...) silently resets it to empty. two results that only differ in images compare equal, so StateFlow/distinctUntilChanged/mutableStateOf will swallow an image-only update. can we override equals, hashCode and toString to include images (binary safe, apiCheck stays green), update the KDoc to say only copy drops it, and add tests for both? if dropping on copy isn't ok, the other option is images in the primary constructor as a major.
| * @property id The opaque identifier of the image. | ||
| * @property passageId The verse or verse range the image illustrates, such as `JHN.3.16` or `1CO.13.4-7`. | ||
| */ | ||
| class SearchImage( |
There was a problem hiding this comment.
needs equals/hashCode/toString on id + passageId. right now two identical images aren't equal, which also breaks the SearchResults equality fix.
| internal data class UnifiedSearchResponse( | ||
| @SerialName("verses") val references: List<VerseSearchResultResponse>, | ||
| @SerialName("topics") val topics: List<TopicSearchResultResponse>, | ||
| @SerialName("images") val images: List<ImageSearchResultResponse> = emptyList(), |
There was a problem hiding this comment.
"images": null still throws and takes verses and topics down with it. make this List<ImageSearchResultResponse?>? = null and map with orEmpty().mapNotNull { it?.searchImage() }, plus tests for null, a null element, and a non-string id.
| var images: List<SearchImage> = emptyList() | ||
| private set | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
needs a KDoc. this is the only way to set images, so say that, and that the 5-arg constructor leaves it empty.
| SearchTopic(id = id, text = text, subtopics = subtopics) | ||
|
|
||
| private fun ImageSearchResultResponse.searchImage(): SearchImage? { | ||
| return SearchImage(id = id ?: return null, passageId = passageId ?: return null) |
There was a problem hiding this comment.
a blank id gets through here. use takeIf { it.isNotBlank() } for both fields like the verse parser does, and cover "" in the malformed test.
| /** | ||
| * An image matching a search, carrying only enough to identify it. | ||
| * | ||
| * @property id The opaque identifier of the image. |
There was a problem hiding this comment.
no need to wait on #218, just say in plain words that this is the id you pass to the verse-image lookup to get the url/size/attribution. same for README:537.
Summary
SearchImage(opaqueid+passageId, a single verse or a range) andSearchResults.imagesfor YPE-6186."images"is now a validfieldsvalue forsearch.unified, so images can come back alone or alongside verses.imagesdefaults to empty when the response omits it (prod/v1doesn't offer images yet), so existing calls keep working.SearchResults.imagesis a body property set through a new constructor, so 2.6.0's constructor,copy,componentNandequalsare unchanged (no major bump).copydoesn't carry images andequalsdoesn't compare them.SearchImageis a plain class, not a data class, so fields can be added later without a major.CONTEXT.mdand the API dump are updated.Test plan
./gradlew spotlessCheck apiCheck testSearchApiTests: images alongside verses and topics, images alone, images omitted by the platform, and a malformed image dropped while its siblings are kept

Confidence Score: 5/5
The PR appears safe to merge; no actionable issues remain.Summary
Adds image results to unified search, with image IDs and passage IDs available through
SearchResults.images.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Unified search request] --> B[Platform response] B --> C[Verse and topic results] B --> D[Image results or empty list] D --> E[Skip images missing an ID or passage] C --> F[SearchResults] E --> FReviews (6) · Last reviewed commit: "fix(core): keep unified search images ad..." · Reviewed by Greptile