add support for onelap#72
Conversation
jat255
left a comment
There was a problem hiding this comment.
Thanks for adding Onelap support! The overall direction is right, but there are a few things to address before merging:
Issues
1. Use Manufacturer.ONELAP.value instead of raw 307
Unlike MYWHOOSH (331), which truly is unknown to the vendored fit_tool, Onelap is defined in the enum as Manufacturer.ONELAP. The raw integer with a comment is inconsistent with every other entry in those lists:
# Both _should_modify_manufacturer and _should_modify_device_info
Manufacturer.ONELAP.value, # instead of: 307, # ONELAP2. Skipping software messages (ID 35) is a global behavior change
if message.global_id == 35:
_logger.debug(f"Skipping Software message at record {i}")
continueThis drops software messages from all platforms, not just Onelap. If Onelap requires this (e.g., their software message causes Garmin Connect to reject the file), it should be gated on the source manufacturer. Dropping this silently for all manufacturers is not acceptable.
3. Broken docstrings
The rewrite_file_id_message and _should_modify_manufacturer docstrings now list only MYWHOOSH and ONELAP as supported manufacturers, omitting DEVELOPMENT, ZWIFT, WAHOO_FITNESS, PEAKSWARE, HAMMERHEAD, and COROS. Looks like a multi-line string was accidentally truncated rather than appended to (maybe an agent mishit when editing?).
4. No tests
The project maintains 100% coverage with a test FIT file per platform. This PR needs a sample Onelap FIT file in tests/files/ and corresponding test cases in test_config.py / test_app.py (at minimum covering AppType.ONELAP and OnelapDetector path detection). The PR will not be accepted until 100% coverage is met.
Minor notes
- The
if message.product is not None:change (vs the old truthiness check) is a real correctness fix -- good catch. OnelapDetectorlooks reasonable. One nit: the docstring lists both macOS and Windows pointing to the same~/Documents/Onelap/Activity/path, which is slightly confusing.
…m patches in tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 1238 1252 +14
=========================================
+ Hits 1238 1252 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.