From 912124c9a1c8d9c9bc76ed93d9f194b9134873f1 Mon Sep 17 00:00:00 2001 From: TShapinsky Date: Thu, 7 Nov 2024 15:28:09 -0700 Subject: [PATCH 1/6] remove most point id requirements --- src/alfalfa/AlfalfaJSON.cpp | 5 ++- src/alfalfa/AlfalfaPoint.cpp | 44 +++++-------------- src/alfalfa/AlfalfaPoint.hpp | 2 +- src/alfalfa/AlfalfaPoint_Impl.hpp | 6 +-- src/alfalfa/test/AlfalfaJSON_GTest.cpp | 59 +++++++------------------- 5 files changed, 32 insertions(+), 84 deletions(-) diff --git a/src/alfalfa/AlfalfaJSON.cpp b/src/alfalfa/AlfalfaJSON.cpp index 518b09946a..edfd8bc02a 100644 --- a/src/alfalfa/AlfalfaJSON.cpp +++ b/src/alfalfa/AlfalfaJSON.cpp @@ -83,9 +83,10 @@ namespace alfalfa { Json::Value AlfalfaJSON_Impl::toJSON() const { Json::Value root; - for (const auto& point : m_points) { + int i = 0; + for (const auto& point : points()) { // No guard here as the toJSON call will throw an exception if the id does not exist. - root[point.id().get()] = point.toJSON(); + root[i++] = point.toJSON(); } return root; } diff --git a/src/alfalfa/AlfalfaPoint.cpp b/src/alfalfa/AlfalfaPoint.cpp index b3dfb30118..67166def8d 100644 --- a/src/alfalfa/AlfalfaPoint.cpp +++ b/src/alfalfa/AlfalfaPoint.cpp @@ -11,8 +11,6 @@ namespace openstudio { namespace alfalfa { - static constexpr std::string_view ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():"; - namespace detail { AlfalfaPoint_Impl::AlfalfaPoint_Impl(const std::string& display_name) { @@ -22,11 +20,7 @@ namespace alfalfa { Json::Value AlfalfaPoint_Impl::toJSON() const { Json::Value point; - if (auto id_ = id()) { - point["id"] = *id_; - } else { - throw std::runtime_error(fmt::format("Point requires a valid ID for export. {}", ID_VALID_CHARS_MSG)); - } + point["id"] = id(); point["name"] = displayName(); if (auto input_ = input()) { point["input"]["type"] = input_->typeName(); @@ -80,24 +74,17 @@ namespace alfalfa { } void AlfalfaPoint_Impl::setId(const std::string& id) { - if (isValidId(id)) { - m_id = id; - } else { - throw std::runtime_error(ID_VALID_CHARS_MSG.data()); + if (id.empty()) { + throw std::runtime_error("Id must have non-zero length"); } + m_id = toIdString(id); } - boost::optional AlfalfaPoint_Impl::id() const { - boost::optional result = m_id; - if (!result.is_initialized()) { - std::string id = toIdString(displayName()); - if (isValidId(id)) { - result = id; - } else { - LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", displayName())); - } + std::string AlfalfaPoint_Impl::id() const { + if (m_id.empty()) { + return toIdString(displayName()); } - return result; + return m_id; } void AlfalfaPoint_Impl::setDisplayName(const std::string& display_name) { @@ -105,11 +92,8 @@ namespace alfalfa { throw std::runtime_error("Display name must have non-zero length"); } m_display_name = display_name; - if (!m_id.is_initialized()) { - const std::string id = toIdString(display_name); - if (!isValidId(id)) { - LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", display_name)); - } + if (m_id.empty()) { + setId(toIdString(display_name)); } } @@ -125,17 +109,11 @@ namespace alfalfa { return m_optional; } - bool AlfalfaPoint_Impl::isValidId(const std::string& id) { - return !id.empty() && boost::regex_match(id, boost::regex(R"(^[A-Za-z0-9_\-\[\]:()]*$)")); - } - std::string AlfalfaPoint_Impl::toIdString(const std::string& str) { return boost::regex_replace(str, boost::regex(" "), "_"); } } // namespace detail - // AlfalfaPoint::AlfalfaPoint(const AlfalfaPoint& point) : m_impl(point.m_impl) {} - AlfalfaPoint::AlfalfaPoint(const std::string& display_name) : m_impl(std::make_shared(display_name)) {} void AlfalfaPoint::setInput(const AlfalfaComponent& component) { @@ -162,7 +140,7 @@ namespace alfalfa { return m_impl->units(); } - boost::optional AlfalfaPoint::id() const { + std::string AlfalfaPoint::id() const { return m_impl->id(); } diff --git a/src/alfalfa/AlfalfaPoint.hpp b/src/alfalfa/AlfalfaPoint.hpp index 3ed3ee5158..1ca63e380a 100644 --- a/src/alfalfa/AlfalfaPoint.hpp +++ b/src/alfalfa/AlfalfaPoint.hpp @@ -69,7 +69,7 @@ namespace alfalfa { /** * Get id of point. By default this will be a version of the display name with spaces removed. */ - boost::optional id() const; + std::string id() const; /** * Set id of point. This is the component which will uniquely identify the point in the API. diff --git a/src/alfalfa/AlfalfaPoint_Impl.hpp b/src/alfalfa/AlfalfaPoint_Impl.hpp index f6d0810a65..bc311434e5 100644 --- a/src/alfalfa/AlfalfaPoint_Impl.hpp +++ b/src/alfalfa/AlfalfaPoint_Impl.hpp @@ -32,7 +32,7 @@ namespace alfalfa { boost::optional units() const; - boost::optional id() const; + std::string id() const; void setId(const std::string& id); @@ -47,13 +47,11 @@ namespace alfalfa { private: static std::string toIdString(const std::string& str); - static bool isValidId(const std::string& id); - boost::optional m_input; boost::optional m_output; std::string m_display_name; boost::optional m_units; - boost::optional m_id; + std::string m_id; bool m_optional = true; // configure logging diff --git a/src/alfalfa/test/AlfalfaJSON_GTest.cpp b/src/alfalfa/test/AlfalfaJSON_GTest.cpp index eca8a69338..f528eef5d8 100644 --- a/src/alfalfa/test/AlfalfaJSON_GTest.cpp +++ b/src/alfalfa/test/AlfalfaJSON_GTest.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include "../AlfalfaJSON.hpp" @@ -194,75 +195,47 @@ TEST(AlfalfaJSON, json_serialization) { const bool parsing_success = Json::parseFromStream(r_builder, ifs, &root, &formatted_errors); EXPECT_TRUE(parsing_success); EXPECT_EQ(alfalfa.toJSON(), root); - for (const AlfalfaPoint& point : alfalfa.points()) { - EXPECT_EQ(root[point.id().get()], point.toJSON()); + int i = 0; + for (const auto& point : alfalfa.points()) { + EXPECT_EQ(root[i++], point.toJSON()); } } TEST(AlfalfaJSON, point_exceptions_logging) { - const std::string ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():"; - const std::string DISPLAY_NAME_VALID_CHARS_MSG = - "Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail."; + const std::string DISPLAY_NAME_EMPTY_ERROR = "Display name must have non-zero length"; + const std::string ID_EMPTY_ERROR = "Id must have non-zero length"; const std::string LOG_CHANNEL = "openstudio.AlfalfaPoint"; StringStreamLogSink ss; ss.setLogLevel(Warn); - const AlfalfaPoint point("Point"); + AlfalfaPoint point("Point"); ASSERT_EQ(0, ss.logMessages().size()); point.id(); ASSERT_EQ(0, ss.logMessages().size()); //Test logging in constructor - AlfalfaPoint invalid_point("Point$$$"); - ASSERT_EQ(1, ss.logMessages().size()); - LogMessage invalid_id_msg = ss.logMessages().at(0); - EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$")); - EXPECT_EQ(invalid_id_msg.logLevel(), Warn); - EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL); - ss.resetStringStream(); - ASSERT_EQ(ss.logMessages().size(), 0); - - // Test logging when getting id() - const boost::optional invalid_point_id = invalid_point.id(); - EXPECT_FALSE(invalid_point_id.is_initialized()); - ASSERT_EQ(ss.logMessages().size(), 1); - invalid_id_msg = ss.logMessages().at(0); - EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$")); - EXPECT_EQ(invalid_id_msg.logLevel(), Warn); - EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL); - ss.resetStringStream(); - ASSERT_EQ(ss.logMessages().size(), 0); - - // Test exception handling in setId() EXPECT_THROW( { try { - invalid_point.setId("Point_123_$$$"); + const AlfalfaPoint invalid_point(""); } catch (const std::runtime_error& error) { - EXPECT_EQ(error.what(), ID_VALID_CHARS_MSG); + EXPECT_EQ(error.what(), DISPLAY_NAME_EMPTY_ERROR); throw; } }, std::runtime_error); - ASSERT_EQ(ss.logMessages().size(), 0); - // Test exception handling in toJSON() + // Test exception handling in setId() EXPECT_THROW( { try { - Json::Value root = invalid_point.toJSON(); + point.setId(""); } catch (const std::runtime_error& error) { - EXPECT_EQ(error.what(), "Point requires a valid ID for export. " + ID_VALID_CHARS_MSG); + EXPECT_EQ(error.what(), ID_EMPTY_ERROR); throw; } }, std::runtime_error); - ASSERT_EQ(ss.logMessages().size(), 1); - invalid_id_msg = ss.logMessages().at(0); - EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$")); - EXPECT_EQ(invalid_id_msg.logLevel(), Warn); - EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL); - ss.resetStringStream(); ASSERT_EQ(ss.logMessages().size(), 0); //Test Calls work when provided with legal input @@ -278,8 +251,7 @@ TEST(AlfalfaJSON, point_exceptions_logging) { // Test that changing display name changes the ID if an ID has not been set. valid_point.setDisplayName("Another Good Point"); ASSERT_EQ(ss.logMessages().size(), 0); - ASSERT_TRUE(valid_point.id().is_initialized()); - ASSERT_EQ(valid_point.id().get(), "Another_Good_Point"); + ASSERT_EQ(valid_point.id(), "Another_Good_Point"); ASSERT_EQ(ss.logMessages().size(), 0); Json::Value valid_json = valid_point.toJSON(); @@ -289,8 +261,7 @@ TEST(AlfalfaJSON, point_exceptions_logging) { const std::string new_id = "Another_Valid_Point(123)"; valid_point.setId(new_id); ASSERT_EQ(ss.logMessages().size(), 0); - ASSERT_TRUE(valid_point.id().is_initialized()); - ASSERT_EQ(valid_point.id().get(), new_id); + ASSERT_EQ(valid_point.id(), new_id); valid_json = valid_point.toJSON(); ASSERT_EQ(ss.logMessages().size(), 0); @@ -298,7 +269,7 @@ TEST(AlfalfaJSON, point_exceptions_logging) { // Test that once an ID is set, setting a new display name won't throw a warning. valid_point.setDisplayName("Valid Name, but Invalid Id $$$"); ASSERT_EQ(ss.logMessages().size(), 0); - ASSERT_EQ(valid_point.id().get(), new_id); + ASSERT_EQ(valid_point.id(), new_id); valid_json = valid_point.toJSON(); ASSERT_EQ(ss.logMessages().size(), 0); From 257d8d3c9dc479d2588e1884321d34535b2ee726 Mon Sep 17 00:00:00 2001 From: TShapinsky Date: Thu, 7 Nov 2024 15:45:29 -0700 Subject: [PATCH 2/6] remove unnecessary setting of point Id --- src/alfalfa/AlfalfaPoint.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/alfalfa/AlfalfaPoint.cpp b/src/alfalfa/AlfalfaPoint.cpp index 67166def8d..b61686f011 100644 --- a/src/alfalfa/AlfalfaPoint.cpp +++ b/src/alfalfa/AlfalfaPoint.cpp @@ -92,9 +92,6 @@ namespace alfalfa { throw std::runtime_error("Display name must have non-zero length"); } m_display_name = display_name; - if (m_id.empty()) { - setId(toIdString(display_name)); - } } std::string AlfalfaPoint_Impl::displayName() const { From 25e61d0bf820e82851e40321c6805c44a0cc5ab4 Mon Sep 17 00:00:00 2001 From: Tobias Shapinsky Date: Fri, 8 Nov 2024 09:05:55 -0700 Subject: [PATCH 3/6] Update src/alfalfa/AlfalfaJSON.cpp Co-authored-by: Julien Marrec --- src/alfalfa/AlfalfaJSON.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/alfalfa/AlfalfaJSON.cpp b/src/alfalfa/AlfalfaJSON.cpp index edfd8bc02a..9fa41f5324 100644 --- a/src/alfalfa/AlfalfaJSON.cpp +++ b/src/alfalfa/AlfalfaJSON.cpp @@ -83,8 +83,7 @@ namespace alfalfa { Json::Value AlfalfaJSON_Impl::toJSON() const { Json::Value root; - int i = 0; - for (const auto& point : points()) { + for (Json::ArrayIndex i = 0; const auto& point : points()) { // No guard here as the toJSON call will throw an exception if the id does not exist. root[i++] = point.toJSON(); } From 175fd88dc304f8d4bda9c7cf6775165262570bc4 Mon Sep 17 00:00:00 2001 From: Tobias Shapinsky Date: Fri, 8 Nov 2024 09:06:33 -0700 Subject: [PATCH 4/6] Update src/alfalfa/test/AlfalfaJSON_GTest.cpp Co-authored-by: Julien Marrec --- src/alfalfa/test/AlfalfaJSON_GTest.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/alfalfa/test/AlfalfaJSON_GTest.cpp b/src/alfalfa/test/AlfalfaJSON_GTest.cpp index f528eef5d8..6a623d7c40 100644 --- a/src/alfalfa/test/AlfalfaJSON_GTest.cpp +++ b/src/alfalfa/test/AlfalfaJSON_GTest.cpp @@ -195,8 +195,7 @@ TEST(AlfalfaJSON, json_serialization) { const bool parsing_success = Json::parseFromStream(r_builder, ifs, &root, &formatted_errors); EXPECT_TRUE(parsing_success); EXPECT_EQ(alfalfa.toJSON(), root); - int i = 0; - for (const auto& point : alfalfa.points()) { + for (Json::ArrayIndex i = 0; const auto& point : alfalfa.points()) { EXPECT_EQ(root[i++], point.toJSON()); } } From 56565e9728178ec5b63fcee2e21b076cf28f9551 Mon Sep 17 00:00:00 2001 From: TShapinsky Date: Fri, 8 Nov 2024 09:13:30 -0700 Subject: [PATCH 5/6] use std::replace instead of regex --- src/alfalfa/AlfalfaPoint.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/alfalfa/AlfalfaPoint.cpp b/src/alfalfa/AlfalfaPoint.cpp index b61686f011..b2584cd767 100644 --- a/src/alfalfa/AlfalfaPoint.cpp +++ b/src/alfalfa/AlfalfaPoint.cpp @@ -7,6 +7,7 @@ #include #include +#include namespace openstudio { namespace alfalfa { @@ -107,7 +108,9 @@ namespace alfalfa { } std::string AlfalfaPoint_Impl::toIdString(const std::string& str) { - return boost::regex_replace(str, boost::regex(" "), "_"); + std::string id_string = str; + std::replace( id_string.begin(), id_string.end(), ' ', '_'); + return id_string; } } // namespace detail From d5c11a6fe496b7ebda2e6ff439b15c292d03b3fb Mon Sep 17 00:00:00 2001 From: TShapinsky Date: Fri, 8 Nov 2024 09:15:45 -0700 Subject: [PATCH 6/6] fix formatting --- src/alfalfa/AlfalfaPoint.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/alfalfa/AlfalfaPoint.cpp b/src/alfalfa/AlfalfaPoint.cpp index b2584cd767..9ffd9a03c5 100644 --- a/src/alfalfa/AlfalfaPoint.cpp +++ b/src/alfalfa/AlfalfaPoint.cpp @@ -109,7 +109,7 @@ namespace alfalfa { std::string AlfalfaPoint_Impl::toIdString(const std::string& str) { std::string id_string = str; - std::replace( id_string.begin(), id_string.end(), ' ', '_'); + std::replace(id_string.begin(), id_string.end(), ' ', '_'); return id_string; } } // namespace detail