Carry the hotlink and download-location URLs on Descriptor - #92
Merged
Conversation
Unsplash's API guidelines require consumers to display photos from the URLs the API returns under "urls" rather than from a copy they host themselves, so that views are attributed to the photographer, and to call the download endpoint when a photo is actually used. Consumers could do neither: Source.Photo had both values in hand and discarded them, keeping only an unexported image with a Filename and a Reader. Descriptor gains ImageURL and DownloadLocation, filled in from the photo already fetched, so no extra request is made. ImageURL prefers the raw variant because it carries no size preset, leaving the caller free to append its own Imgix parameters, and falls back to full then regular when a response omits it. The two are deliberately separate fields. Displaying a photo is not a download, and a consumer wanting to follow the guidelines has to trigger them at different moments — the hotlink on every render, the download once when the photo is chosen. Both are omitted when empty, so photo metadata written by earlier versions still loads unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the pkg/photo metadata model and the Unsplash implementation to retain Unsplash’s “hotlink” image URL and the “download_location” URL so downstream consumers (e.g., openscripture.today) can comply with Unsplash API guidelines without re-querying the API.
Changes:
- Add
ImageURLandDownloadLocationtophoto.Descriptor(YAML/JSONomitempty). - Populate those fields in
unsplash.Source.Photo(including URL-variant preference logic) and add targeted tests for selection/fallback behavior. - Bump version to
1.1.0and record the change inChanges.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/photo/unsplash/photo.go | Adds hotlinkURL helper and populates Descriptor.ImageURL + Descriptor.DownloadLocation from the Unsplash response. |
| pkg/photo/unsplash/photo_test.go | Expands Unsplash fixture and adds a table test to verify URL variant preference/fallback behavior. |
| pkg/photo/photo.go | Extends Descriptor with exported ImageURL and DownloadLocation fields with serialization tags. |
| cmd/version.txt | Bumps project version to 1.1.0. |
| Changes.md | Adds 1.1.0 TBD changelog entry describing the new descriptor fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
15
to
+21
| // stringValue is a helper for use with the Source Client to pull out strings | ||
| // from responses. | ||
| // hotlinkURL picks the URL to display the photo from. Unsplash asks that photos | ||
| // be shown from the URLs it returns under "urls" rather than from a copy the | ||
| // consumer hosts. Raw is preferred because it carries no size preset, leaving | ||
| // the caller free to append its own Imgix parameters; full and regular stand in | ||
| // when a response omits it. |
hotlinkURL was inserted between stringValue's doc comment and stringValue itself, so the comment described the wrong function and stringValue was left undocumented. Moves hotlinkURL below the two generic string helpers, which restores the file's original grouping -- the small pointer-dereference helpers first, then the domain functions -- and puts hotlinkURL after urlValueString, which it calls. Addresses Copilot review feedback on #92. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Needed by openscripture.today, which is trying to certify as an Unsplash production application and currently fails two of their checklist items.
Why
Unsplash's API guidelines require that consumers:
photo.urls, rather than from a copy they host themselves — this is how views get attributed to the photographer.photo.links.download_location) when a photo is actually used.A consumer of this package could do neither.
Source.Photohad both values in hand and threw them away, keeping only an unexportedunsplashImageexposingFilename()andReader(). The only way out was to re-query the API for data that had already been fetched.What changed
photo.Descriptorgains two fields, populated inunsplash.Source.Photofrom the response it already makes — no additional API request:ImageURLprefers therawvariant, which carries no size preset and leaves the caller free to append its own Imgix parameters, falling back tofullthenregularwhen a response omits it.The two are deliberately separate fields rather than one. Displaying a photo is not a download, and a consumer following the guidelines has to trigger them at different moments — the hotlink on every render, the download once when the photo is chosen. Collapsing them would make that impossible to express.
Compatibility
Both fields are
omitempty, so photo metadata written by earlier versions loads unchanged and re-serializes without them. Nothing else in the struct moved.Testing
The existing
TestSourcefixture now includes theurlsanddownload_locationthe real API returns, and asserts both new fields. A new table test covers the variant preference order and the cases where a response has nourlsblock or an empty one —hotlinkURLis at 100% statement coverage.go build,go test ./...,golangci-lint run ./...(0 issues), and the coverage gate all pass — total is 84.5% against the 80% floor.Changelog and
cmd/version.txtbumped to1.1.0 TBDper the release process.🤖 Generated with Claude Code