Fix GPSTimeStamp for fractional seconds stored as low-denominator rationals - #56
Closed
tonimelisma wants to merge 1 commit into
Closed
Fix GPSTimeStamp for fractional seconds stored as low-denominator rationals#56tonimelisma wants to merge 1 commit into
tonimelisma wants to merge 1 commit into
Conversation
The convertToTimestampString converter used ratNum() (numerator only) for
all three time components. It had a follow-up hack that detected 10-char
results and inserted a decimal point:
4279/100 → numerator 4279 → "13:03:4279" (10 chars) → "13:03:42.79"
This only works when the seconds numerator is exactly 4 digits. A rational
like 181/5 (= 36.2 seconds) has a 3-digit numerator and falls through:
181/5 → numerator 181 → "22:22:181" → no decimal inserted
Fix: use toFloat64() and strconv.FormatFloat for the seconds component.
Integer seconds still produce "47"; fractional seconds produce "36.2".
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
|
This needs to fix a failing test case. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 79.38% 79.40% +0.01%
==========================================
Files 14 14
Lines 1606 1607 +1
==========================================
+ Hits 1275 1276 +1
Misses 228 228
Partials 103 103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
|
I'll handle this in #58 |
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.
The
convertToTimestampStringconverter inhelpers.gousedratNum()(numeratoronly) for all three components, then formatted with
%02d. It had a follow-up hackthat detected 10-character results and inserted a decimal point, e.g.:
This only works when the seconds numerator happens to be exactly 4 digits. A rational
like 181/5 (= 36.2 seconds) has a 3-digit numerator and falls through:
Fix: compute the seconds value properly using
toFloat64()(already used elsewherefor GPS coordinate conversion) and format with
strconv.FormatFloat(secs, 'f', -1, 64).Integer seconds (e.g. 47/1) still produce "47" with no trailing decimal; fractional
seconds (e.g. 181/5) produce "36.2".
The existing
goexif/has-lens-info.jpggolden test (seconds = 4279/100 = 42.79)continues to pass as before.
This bug was discovered while adding HEIF support and testing against an iPhone 15 Pro
photo whose GPS log records seconds as 181/5.