Show the psnr reported by optimize and convert - #45
Merged
Conversation
The API now returns psnr for optimize and convert. When it is present the human summary gains a ", PSNR 41.27 dB" suffix and the --json output gains a psnr key; when it is null (resize, thumbnail, no measurable loss, older API) the output is unchanged. Requires mathiasgrimm/glimpse-php ^3.1 for the ImageResult psnr property; the lock stays on 3.0.0 until v3.1.0 is tagged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Format the PSNR summary with %.2F so a non-C LC_NUMERIC cannot turn the decimal point into a comma. - Lock the "resize carries no psnr" contract with an exact --json test on the resize command, and assert the human summary has no PSNR when the API reports null. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Two independent reviews, one by a Claude agent and one by codex (gpt-5.6-sol, read-only). Both approve the behavior; codex requested one change (locale-dependent formatting). Addressed in commit d39c2fd, followed by both reviews. What was addressed
Left as noted follow-ups (not this PR): the SDK-version bump means CI stays red until glimpse-php v3.1.0 is tagged and the lock is refreshed ( Claude reviewVerdict: clean and correct; approve once glimpse-php v3.1.0 is tagged and the lock is refreshed. What I verified
Findings
Codex reviewVerdict: Changes requested. The JSON behavior and STDERR routing are correct, but human output is locale-dependent. Medium
Low
Minor notes
|
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.
CLI side of Art-Commerce-Systems/glimpseimg.com#91 (API PR: Art-Commerce-Systems/glimpseimg.com#98, SDK PR: mathiasgrimm/glimpse-php#11).
What
GlimpseCommand::emit()appends, PSNR 41.27 dBto the human summary and adds apsnrkey to--jsonoutput, but only when the SDK reports a non-null value. Resize and thumbnail (and older APIs) yield null from the SDK, so their output is byte-identical to today, locked by the existing exact-match JSON test plus a new null-omission test.fakeTransformResponse()/fakeTransform()gain an overrides parameter, following thefakeInfoResponsemerge pattern.composer.jsonbumpsmathiasgrimm/glimpse-phpto^3.1(theImageResult->psnrproperty).Merge order (CI is red until then, expected)
composer update mathiasgrimm/glimpse-php, commit the lock, CI goes green, merge, release v1.4.0 (make releasebundles the updated vendor into the phar).Until step 2 the locked SDK 3.0.0 has no
psnrproperty, so the new tests fail on CI. Locally the suite was run against the SDK branch and passes: 288 tests, 800 assertions, Pint and PHPStan clean.Follow-up (out of scope, per the plan on the issue): persisting psnr into the baseline file.
🤖 Generated with Claude Code