Conversation
elshize
force-pushed
the
stats-builder
branch
5 times, most recently
from
4abf7eb to
1bc3e75
Compare
elshize
force-pushed
the
stats-builder
branch
from
1bc3e75 to
1976266
Compare
include/pisa/util/stats_builder.hpp
Outdated
| */ | ||
| class StatsBuilder { | ||
| private: | ||
| std::unique_ptr<::pisa::detail::StatsBuilderInterface> m_impl; |
There was a problem hiding this comment.
Separating implementation details to the source file so that we can keep nlohmann dependency private.
src/util/stats_builder.cpp
Outdated
| virtual void add(std::string const& key, double value) { m_json[key] = value; } | ||
| virtual void add(std::string const& key, std::string value) { m_json[key] = std::move(value); } | ||
| virtual void add(std::string const& key, const char* value) { m_json[key] = value; } | ||
| [[nodiscard]] virtual auto build() const -> std::string { return m_json.dump(2); } |
There was a problem hiding this comment.
I think build sounds as something that becomes immutable (or finalized) once it is "built". But in this case, I can build it and then continue adding elements to the structure. Maybe a better name could be str/to_str?
As a follow-up, if we make this change, we could also consider renaming pisa::stats_builder() by pisa::json_stats.
There was a problem hiding this comment.
Thanks, I think this makes sense, I'll make the suggested changes once I find a moment.
src/util/stats_builder.cpp
Outdated
|
|
||
| namespace pisa { | ||
|
|
||
| class JsonStatsBuilder: public ::pisa::detail::StatsBuilderInterface { |
There was a problem hiding this comment.
I think JsonStatsBuilder sounds verbose (very Java-style). Maybe JsonStats could be enough? That would make sense, since we could expose a str method instead of having a build method (comment in line 31).
|
@elshize, looks good to me! |
2 participants
No description provided.