feat: support more map key type#67
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
- Coverage 57.17% 56.40% -0.78%
==========================================
Files 16 16
Lines 439 445 +6
Branches 82 82
==========================================
Hits 251 251
- Misses 138 144 +6
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@injae |
The reason Codecov failed seems to be that the test coverage dropped compared to before. |
| namespace detail { | ||
| template <typename T> | ||
| std::string to_string(const T& key) { | ||
| if constexpr (std::is_arithmetic_v<T>) { | ||
| return std::to_string(key); | ||
| } else { | ||
| std::ostringstream oss; | ||
| oss << key; | ||
| return oss.str(); | ||
| } | ||
| } | ||
| } | ||
| template <typename K, typename = void> | ||
| struct from_string { | ||
| static K from(const std::string& key) { | ||
| K result; | ||
| std::stringstream ss(key); | ||
| ss >> result; | ||
| if (ss.fail()) { | ||
| throw std::runtime_error("Cannot convert JSON key to type " + std::string(typeid(K).name())); | ||
| } | ||
| return result; | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| template <> | ||
| struct from_string<std::string> { | ||
| static std::string from(const std::string& key) { | ||
| return key; | ||
| } | ||
| }; | ||
|
|
||
| template <typename K> | ||
| struct from_string<K, std::enable_if_t<std::is_integral_v<K> && !std::is_same_v<K, bool>>> { | ||
| static K from(const std::string& key) { | ||
| return static_cast<K>(std::stoll(key)); | ||
| } | ||
| }; | ||
|
|
||
| template <> | ||
| struct from_string<bool> { | ||
| static bool from(const std::string& key) { | ||
| if (key == "true" || key == "1") return true; | ||
| if (key == "false" || key == "0") return false; | ||
| throw std::invalid_argument("Invalid boolean key: " + key); | ||
| } | ||
| }; | ||
|
|
||
| template <typename K> | ||
| struct from_string<K, std::enable_if_t<std::is_enum_v<K>>> { | ||
| static K from(const std::string& key) { | ||
| return static_cast<K>(std::stoi(key)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Instead of using that specialization, it would be better to use Serde’s serialize and deserialize.
| constexpr inline static auto from(serde_ctx& ctx, T& data, std::string_view key) { | ||
| using key_type = typename T::key_type; | ||
| using new_T = std::unordered_map<std::string, typename T::mapped_type>; | ||
| new_T map_data; | ||
| serde_adaptor<typename serde_ctx::Adaptor, new_T, type::map_t>::from(ctx.adaptor, key, map_data); | ||
| std::transform(map_data.begin(), map_data.end(), std::inserter(data, data.begin()), | ||
| [](const auto& pair) { | ||
| return std::make_pair(from_string<key_type>::from(pair.first), pair.second); | ||
| }); | ||
| ctx.read(); | ||
| } | ||
| constexpr inline static auto into(serde_ctx& ctx, const T& data, std::string_view key) { | ||
|
|
||
| using value_type = typename T::mapped_type; | ||
| using new_T = std::unordered_map<std::string, value_type>; | ||
| new_T map_data; | ||
| std::transform(data.begin(), data.end(), std::inserter(map_data, map_data.begin()), | ||
| [](const auto& pair) { | ||
| return std::make_pair(detail::to_string(pair.first), pair.second); | ||
| }); | ||
| serde_adaptor<typename serde_ctx::Adaptor, new_T, type::map_t>::into(ctx.adaptor, key, map_data); |
There was a problem hiding this comment.
It seems that we may need to revisit the current approach for implementing T <-> string conversions in the serde core.
There appears to be a small issue with the direction I suggested previously. After giving it some more thought, I think the approach where each adapter is responsible for its own implementation—similar to the adapter you implemented in the previous PR—might be a better and cleaner solution overall.
With the current approach, I see a couple of concerns:
-
Implementing this would require introducing STL containers into the serdepp core.
I do understand why using
unordered_mapis effectively unavoidable in this case
(according to the C++ specification, whenT = M<K, V>, we can extractKandV, but notMitself, which makes it impossible to construct something likeM<string, V>).That said, serdepp currently tries to intentionally avoid using STL containers in the core library.
In the C++ ecosystem, it’s fairly common to rely on spec-compatible custom implementations instead of STL containers, often due to performance considerations. -
This approach would make serdepp less adapter-neutral.
Serdepp was originally designed with the goal of supporting adapters that are not necessarily JSON-compatible.
From that perspective, implementing it this way might go slightly against that original intention.
In summary:
It might be better to delegate the responsibility for T <-> key support to the adapter itself.
Rather than adding a separate to_string specialization in the core, we could rely on the existing serde serialize and deserialize implementations that adapters already provide.
Additionally, it could be helpful to introduce shared utility functions for this behavior, so that different adapters can reuse the same logic where appropriate.
Additionally, since we’re currently using squash merges, it should be fine to make the changes in this PR.
Refactor serialization and deserialization functions for improved type handling.
2 participants
fix bug
#64 (comment)