DRY conversions / move convert unit tests to one spot#2
Merged
heathhenley merged 3 commits intomainfrom Feb 13, 2026
Merged
Conversation
It was easy to get started but makes more sense - moving all the tests into a single place next - subscriber and request tests will eventually test more of the actual functionality instead of conversion internals.
Move all conversion tests to one place Autoformat with clang format
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicate conversion code from subscriber.cpp and requests.cpp into a single shared implementation in conversions.cpp, following the DRY (Don't Repeat Yourself) principle. The refactoring moves all proto-to-wrapper type conversions into the farsounder::detail namespace and consolidates all conversion tests into a single test file with improved coverage.
Changes:
- Created new
conversions.cppandconversions_internal.hppto centralize all conversion logic previously duplicated across subscriber and requests modules - Consolidated two separate test files (
subscriber_conversion_tests.cppandrequests_conversion_tests.cpp) into one comprehensiveconversions_unit_tests.cppwith additional edge case coverage - Updated all call sites to use fully qualified namespace references (
farsounder::detail::) where needed to avoid ambiguity
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/conversions.cpp | New file containing all consolidated conversion implementations (timestamp, enums, data structures) |
| src/conversions_internal.hpp | New header declaring all conversion functions in farsounder::detail namespace |
| src/subscriber.cpp | Removed duplicate conversion implementations and unnecessary <ctime> include |
| src/subscriber_internal.hpp | Simplified to just include conversions_internal.hpp |
| src/requests.cpp | Removed duplicate conversion implementations, updated calls to use farsounder::detail:: qualification |
| src/requests_internal.hpp | Removed conversion function declarations (now in conversions_internal.hpp) |
| tests/conversions_unit_tests.cpp | New consolidated test file with improved coverage including edge cases for unknown enum values and missing optional fields |
| tests/CMakeLists.txt | Updated to reference new consolidated test file |
| CMakeLists.txt | Added conversions.cpp to library sources |
| examples/basic_client.cpp | Minor formatting improvements to break long lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
No description provided.