muktihari / fit

A FIT SDK for decoding and encoding Garmin FIT files in Go supporting FIT Protocol V2.
BSD 3-Clause "New" or "Revised" License
42 stars 4 forks source link

fix: segment file can have multiple leaderboard entries #470

Closed drey closed 1 month ago

drey commented 1 month ago

Hi folks!

Thank you for pretty handy SDK. I found that currently a segment file can have the only one leaderboard entry. This is not expected behaviour. Multiple leaders should be supported by design. In the case of Strava segment it is quite often that a segment has atleast three leaderboard entries like KOM, QOM and Personal Best.

This PR makes possible to add multiple leaderboard entries to a segment file.

Keep going!

Regards, Ilya

muktihari commented 1 month ago

Hi, thanks for contributing, nice catch on segment leaderboard entry! I can confirm that it should have been multiple values after exporting segment from Strava. I have minor request on the field naming, since it's now a slice, can you rename it into its plural form, SegmentLeaderboardEntries will do.

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.94%. Comparing base (06080c4) to head (f553e43).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #470 +/- ## ======================================= Coverage 99.94% 99.94% ======================================= Files 42 42 Lines 3689 3689 ======================================= Hits 3687 3687 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

drey commented 1 month ago

@muktihari, good catch! Done.

muktihari commented 1 month ago

Sorry @Drey , one more thing, I didn't see it earlier since it was minimized in code diff. On line 65 - 68:

var size = 4 // non slice fields
size += len(f.SegmentPoints) + len(f.DeveloperDataIds) +
    len(f.FieldDescriptions) + len(f.UnrelatedMessages)

Please change it to:

var size = 3 // non slice fields
size += len(f.SegmentPoints) + len(f.SegmentLeaderboardEntries) + len(f.DeveloperDataIds) +
    len(f.FieldDescriptions) + len(f.UnrelatedMessages)

By calculating the correct slice's len, this will prevent runtime to reallocate the resulting messages. Other changes look good to me, I will merge it after this change. Thanks!

drey commented 1 month ago

@muktihari, no worries, it's my bad. Done.

muktihari commented 1 month ago

Merged, I appreciate the work. Thanks!