feat: --gif follow-up polish (#32)#34
Merged
Merged
Conversation
…ext nudge) Addresses the non-blocking follow-ups from the --gif whole-branch review: - JSON-mode warning surfacing: uploadResult gains a `warning` field, so a --json consumer (stderr suppressed) still learns a GIF was capped, oversized, or renamed. Populated from the assembleGIF warning. - Re-encode-failure fallback: when only the size-ceiling reduction encode errors, keep the valid oversized first encode and warn, instead of failing and discarding it. - --name without an inline-image extension now gets `.gif` appended (with a warning) so the clip renders inline rather than as a click-through link; gh.IsImage exported for the check. - Tests: both-guards-in-one-call, re-encode-failure fallback (via an injectable reduction-encode seam), name-ext append + unchanged-when-valid, and the JSON vs stderr warning-surfacing branches through runWithDeps. The shared name-sanitization helper item is intentionally not taken: the assembleGIF sanitize-with-fallback contract and validateName's validate-and-error contract differ, so a shared helper would couple two deliberately independent defenses for marginal gain. Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
5 tasks
Remove 7 test doc/inline comments that restated the test name or its body; keep the two that explain non-obvious fixture values (why the frame/ceiling constants are chosen, why the kept-frame assertion is 4). Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
Council review adopted: - --name now normalizes to a .gif extension (case-insensitive) rather than accepting any inline-image extension. The payload is always a GIF and GitHub derives the content-type from the name extension, so a GIF shipped under a .png/.jpg name can render as a static still instead of animating — the exact case --gif exists to prevent. Drops the gh.IsImage export (only this guard used it). - The size-ceiling reduction encoder is injected via a nil-defaulting gifAssembleOptions field instead of a mutable package var, scoping the test override per call so it can't race a future t.Parallel test. Tests updated: .gif (incl. uppercase .GIF) left unchanged; non-.gif image name normalized to .gif; re-encode-failure seam driven through the option. Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
Substituting filepath.Ext dropped legitimate dotted base-name segments — "release-1.0" became "release-1.gif". Append ".gif" instead so every segment is preserved while the payload still ships under a .gif name that animates inline. Still case-insensitive, so an existing .GIF is untouched. Test renamed and extended to cover the dotted-name case. Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
- No --name resolves to "clip.gif" (not a frame basename), so the .gif normalization is a no-op and emits no --name warning on the common path. - The size-ceiling reduction success path emits its "reduced to …" warning (driven via the injected re-encode returning a small payload). Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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
1 participant
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.
Closes #32 (4 of 5 items; item 5 intentionally declined — see below).
Non-blocking polish from the
--gifwhole-branch review, plus fixes adopted from this PR's own council review.Changes
uploadResultgains awarningfield (omitempty). In--jsonmode stderr is suppressed, so this is the only channel a consumer has to learn the GIF was capped / oversized / renamed.gifAssembleOptionsfield (per-call seam, no racy package global).--nameforced to.gif— the payload is always a GIF and GitHub derives content-type from the name extension, so a non-.gifname (even.png) can render as a static still instead of animating..gifis appended (case-insensitively;.GIFuntouched) rather than substituted, so a dotted base name likerelease-1.0keeps its.0..gif/.GIFunchanged, non-.gifappended, dotted-name preserved; no---namedefaults toclip.gifwith no spurious warning; JSON vs stderr warning branches throughrunWithDeps.Item 5 (shared name-sanitization helper) — declined
The issue notes the two checks are "kept separate deliberately as defense-in-depth."
assembleGIFsanitizes-with-fallback (filepath.Base+ reject./../separator);validateNamevalidates-and-errors. Different contracts — a shared helper would couple two deliberately independent defenses for marginal gain.Verification
go build ./...,go vet ./...— clean.go test ./...— all pass.golangci-lint run ./...— 0 issues.go.yml, dispatched on the branch): Go Test, Go Lint, License Check all green.https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B