From 4faab460691c683b8671d5305ee17c9ef8dd170a Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 22 May 2021 21:01:09 +0100 Subject: [PATCH] Make MessageContent::Choose() return an optional And propagate this through the File, Message and PluginCleaningData APIs that call it. --- include/loot/metadata/file.h | 3 +- include/loot/metadata/message.h | 10 +++--- include/loot/metadata/message_content.h | 5 +-- include/loot/metadata/plugin_cleaning_data.h | 2 +- src/api/metadata/file.cpp | 2 +- src/api/metadata/message.cpp | 17 +++++++--- src/api/metadata/message_content.cpp | 15 ++++++--- src/api/metadata/plugin_cleaning_data.cpp | 2 +- src/api/metadata/plugin_metadata.cpp | 15 +++++---- src/tests/api/internals/metadata/file_test.h | 6 ++-- .../internals/metadata/message_content_test.h | 33 ++++++++++--------- .../api/internals/metadata/message_test.h | 13 ++++---- .../metadata/plugin_cleaning_data_test.h | 12 +++---- 13 files changed, 76 insertions(+), 59 deletions(-) diff --git a/include/loot/metadata/file.h b/include/loot/metadata/file.h index 471c99d2..97016d18 100644 --- a/include/loot/metadata/file.h +++ b/include/loot/metadata/file.h @@ -105,7 +105,8 @@ public: * @return The MessageContent object for the preferred language, or if one * does not exist, the English-language MessageContent object. */ - LOOT_API MessageContent ChooseDetail(const std::string& language) const; + LOOT_API std::optional ChooseDetail( + const std::string& language) const; private: Filename name_; diff --git a/include/loot/metadata/message.h b/include/loot/metadata/message.h index d093c6cf..29a01585 100644 --- a/include/loot/metadata/message.h +++ b/include/loot/metadata/message.h @@ -109,7 +109,7 @@ public: * @return A MessageContent object for the preferred language, or for English * if a MessageContent object is not available for the given language. */ - LOOT_API MessageContent GetContent(const std::string& language) const; + LOOT_API std::optional GetContent(const std::string& language) const; /** * Get the message as a SimpleMessage given a language. @@ -118,7 +118,7 @@ public: * @return A SimpleMessage object for the preferred language, or for English * if message text is not available for the given language. */ - LOOT_API SimpleMessage ToSimpleMessage(const std::string& language) const; + LOOT_API std::optional ToSimpleMessage(const std::string& language) const; private: MessageType type_; @@ -133,7 +133,7 @@ LOOT_API bool operator!=(const Message& lhs, const Message& rhs); /** * Check if the first Message object is greater than the second Message object. - * @returns True if the second Message object is less than the first Message + * @returns True if the second Message object is less than the first Message * object, false otherwise. */ LOOT_API bool operator>(const Message& lhs, const Message& rhs); @@ -141,13 +141,13 @@ LOOT_API bool operator>(const Message& lhs, const Message& rhs); /** * Check if the first Message object is less than or equal to the second * Message object. - * @returns True if the first Message object is not greater than the second + * @returns True if the first Message object is not greater than the second * Message object, false otherwise. */ LOOT_API bool operator<=(const Message& lhs, const Message& rhs); /** - * Check if the first Message object is greater than or equal to the second + * Check if the first Message object is greater than or equal to the second * Message object. * @returns True if the first Message object is not less than the second * Message object, false otherwise. diff --git a/include/loot/metadata/message_content.h b/include/loot/metadata/message_content.h index 4ab66c98..6cab6319 100644 --- a/include/loot/metadata/message_content.h +++ b/include/loot/metadata/message_content.h @@ -26,6 +26,7 @@ #include #include +#include #include "loot/api_decorator.h" @@ -108,9 +109,9 @@ public: * If no locale or language code matches are found and content in the * default language is present, that content is returned. * - * Otherwise, a default-constructed MessageContent is returned. + * Otherwise, an empty optional is returned. */ - LOOT_API static MessageContent Choose( + LOOT_API static std::optional Choose( const std::vector content, const std::string& language); diff --git a/include/loot/metadata/plugin_cleaning_data.h b/include/loot/metadata/plugin_cleaning_data.h index dbe4939f..df92bd71 100644 --- a/include/loot/metadata/plugin_cleaning_data.h +++ b/include/loot/metadata/plugin_cleaning_data.h @@ -144,7 +144,7 @@ public: * @return The MessageContent object for the preferred language, or if one * does not exist, the English-language MessageContent object. */ - LOOT_API MessageContent ChooseDetail(const std::string& language) const; + LOOT_API std::optional ChooseDetail(const std::string& language) const; private: uint32_t crc_; diff --git a/src/api/metadata/file.cpp b/src/api/metadata/file.cpp index 18006aaf..6e733ae9 100644 --- a/src/api/metadata/file.cpp +++ b/src/api/metadata/file.cpp @@ -84,7 +84,7 @@ std::string File::GetDisplayName() const { std::vector File::GetDetail() const { return detail_; } -MessageContent File::ChooseDetail(const std::string& language) const { +std::optional File::ChooseDetail(const std::string& language) const { return MessageContent::Choose(detail_, language); } diff --git a/src/api/metadata/message.cpp b/src/api/metadata/message.cpp index ad548965..259e86cf 100644 --- a/src/api/metadata/message.cpp +++ b/src/api/metadata/message.cpp @@ -86,16 +86,23 @@ bool Message::operator==(const Message& rhs) const { MessageType Message::GetType() const { return type_; } std::vector Message::GetContent() const { return content_; } -MessageContent Message::GetContent(const std::string& language) const { + +std::optional Message::GetContent( + const std::string& language) const { return MessageContent::Choose(content_, language); } -SimpleMessage Message::ToSimpleMessage(const std::string& language) const { - MessageContent content = GetContent(language); +std::optional Message::ToSimpleMessage( + const std::string& language) const { + auto content = GetContent(language); + if (!content.has_value()) { + return std::nullopt; + } + SimpleMessage simpleMessage; simpleMessage.type = GetType(); - simpleMessage.language = content.GetLanguage(); - simpleMessage.text = content.GetText(); + simpleMessage.language = content.value().GetLanguage(); + simpleMessage.text = content.value().GetText(); simpleMessage.condition = GetCondition(); return simpleMessage; diff --git a/src/api/metadata/message_content.cpp b/src/api/metadata/message_content.cpp index 94970f2b..0fe4134d 100644 --- a/src/api/metadata/message_content.cpp +++ b/src/api/metadata/message_content.cpp @@ -57,10 +57,11 @@ bool MessageContent::operator==(const MessageContent& rhs) const { return text_ == rhs.text_ && language_ == rhs.language_; } -MessageContent MessageContent::Choose(const std::vector content, +std::optional MessageContent::Choose( + const std::vector content, const std::string& language) { if (content.empty()) - return MessageContent(); + return std::nullopt; else if (content.size() == 1) return content[0]; else { @@ -68,7 +69,7 @@ MessageContent MessageContent::Choose(const std::vector content, auto isCountryCodeGiven = languageCode.length() != language.length(); std::optional matchedLanguage; - MessageContent english; + std::optional english; for (const auto& mc : content) { auto contentLanguage = mc.GetLanguage(); @@ -94,10 +95,14 @@ MessageContent MessageContent::Choose(const std::vector content, } if (matchedLanguage.has_value()) { - return matchedLanguage.value(); + return matchedLanguage; } - return english; + if (english.has_value()) { + return english; + } + + return std::nullopt; } } diff --git a/src/api/metadata/plugin_cleaning_data.cpp b/src/api/metadata/plugin_cleaning_data.cpp index 074b472d..6d790a15 100644 --- a/src/api/metadata/plugin_cleaning_data.cpp +++ b/src/api/metadata/plugin_cleaning_data.cpp @@ -117,7 +117,7 @@ std::vector PluginCleaningData::GetDetail() const { return detail_; } -MessageContent PluginCleaningData::ChooseDetail( +std::optional PluginCleaningData::ChooseDetail( const std::string& language) const { return MessageContent::Choose(detail_, language); } diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp index c7dafaa4..c0ea236e 100644 --- a/src/api/metadata/plugin_metadata.cpp +++ b/src/api/metadata/plugin_metadata.cpp @@ -140,13 +140,14 @@ std::vector PluginMetadata::GetLocations() const { std::vector PluginMetadata::GetSimpleMessages( const std::string& language) const { - std::vector simpleMessages(messages_.size()); - std::transform(begin(messages_), - end(messages_), - begin(simpleMessages), - [&](const Message& message) { - return message.ToSimpleMessage(language); - }); + std::vector simpleMessages; + + for (auto message : messages_) { + auto simpleMessage = message.ToSimpleMessage(language); + if (simpleMessage.has_value()) { + simpleMessages.push_back(simpleMessage.value()); + } + } return simpleMessages; } diff --git a/src/tests/api/internals/metadata/file_test.h b/src/tests/api/internals/metadata/file_test.h index 836f3e9d..f515e370 100644 --- a/src/tests/api/internals/metadata/file_test.h +++ b/src/tests/api/internals/metadata/file_test.h @@ -373,7 +373,7 @@ TEST(File, chooseDetailShouldReturnTheGivenLanguageMessageContentIfItExists) { MessageContent("french", "fr")}; File file("", "", "", content); - EXPECT_EQ(content[1], file.ChooseDetail("fr")); + EXPECT_EQ(content[1], file.ChooseDetail("fr").value()); } TEST( @@ -383,7 +383,7 @@ TEST( MessageContent("french", "fr")}; File file("", "", "", content); - EXPECT_EQ(content[0], file.ChooseDetail("de")); + EXPECT_EQ(content[0], file.ChooseDetail("de").value()); } TEST( @@ -393,7 +393,7 @@ TEST( MessageContent("french", "fr")}; File file("", "", "", content); - EXPECT_EQ(MessageContent(), file.ChooseDetail("es")); + EXPECT_FALSE(file.ChooseDetail("es").has_value()); } TEST(File, emittingAsYamlShouldSingleQuoteValues) { diff --git a/src/tests/api/internals/metadata/message_content_test.h b/src/tests/api/internals/metadata/message_content_test.h index b24a300a..bff9ffe1 100644 --- a/src/tests/api/internals/metadata/message_content_test.h +++ b/src/tests/api/internals/metadata/message_content_test.h @@ -240,11 +240,10 @@ TEST( } TEST(MessageContent, - chooseShouldReturnAnEmptyEnglishMessageIfTheVectorIsEmpty) { + chooseShouldReturnANulloptIfTheVectorIsEmpty) { auto content = MessageContent::Choose(std::vector(), "fr"); - EXPECT_EQ("en", content.GetLanguage()); - EXPECT_EQ("", content.GetText()); + EXPECT_FALSE(content.has_value()); } TEST(MessageContent, chooseShouldReturnTheOnlyElementOfASingleElementVector) { @@ -261,8 +260,7 @@ TEST( MessageContent("test2", "fr")}; auto content = MessageContent::Choose(contents, "pt"); - EXPECT_EQ("en", content.GetLanguage()); - EXPECT_EQ("", content.GetText()); + EXPECT_FALSE(content.has_value()); } TEST(MessageContent, @@ -274,8 +272,9 @@ TEST(MessageContent, MessageContent("test5", "pt_BR")}; auto content = MessageContent::Choose(contents, "pt_BR"); - EXPECT_EQ("pt_BR", content.GetLanguage()); - EXPECT_EQ("test5", content.GetText()); + EXPECT_TRUE(content.has_value()); + EXPECT_EQ("pt_BR", content.value().GetLanguage()); + EXPECT_EQ("test5", content.value().GetText()); } TEST( @@ -287,8 +286,9 @@ TEST( MessageContent("test4", "pt")}; auto content = MessageContent::Choose(contents, "pt_BR"); - EXPECT_EQ("pt", content.GetLanguage()); - EXPECT_EQ("test4", content.GetText()); + EXPECT_TRUE(content.has_value()); + EXPECT_EQ("pt", content.value().GetLanguage()); + EXPECT_EQ("test4", content.value().GetText()); } TEST( @@ -299,8 +299,9 @@ TEST( MessageContent("test3", "pt_PT")}; auto content = MessageContent::Choose(contents, "pt_BR"); - EXPECT_EQ("en", content.GetLanguage()); - EXPECT_EQ("test1", content.GetText()); + EXPECT_TRUE(content.has_value()); + EXPECT_EQ("en", content.value().GetLanguage()); + EXPECT_EQ("test1", content.value().GetText()); } TEST( @@ -314,8 +315,9 @@ TEST( }; auto content = MessageContent::Choose(contents, "pt"); - EXPECT_EQ("pt", content.GetLanguage()); - EXPECT_EQ("test4", content.GetText()); + EXPECT_TRUE(content.has_value()); + EXPECT_EQ("pt", content.value().GetLanguage()); + EXPECT_EQ("test4", content.value().GetText()); } TEST( @@ -327,8 +329,9 @@ TEST( MessageContent("test4", "pt_BR")}; auto content = MessageContent::Choose(contents, "pt"); - EXPECT_EQ("pt_PT", content.GetLanguage()); - EXPECT_EQ("test3", content.GetText()); + EXPECT_TRUE(content.has_value()); + EXPECT_EQ("pt_PT", content.value().GetLanguage()); + EXPECT_EQ("test3", content.value().GetText()); } TEST(MessageContent, emittingAsYamlShouldOutputDataCorrectly) { diff --git a/src/tests/api/internals/metadata/message_test.h b/src/tests/api/internals/metadata/message_test.h index 782ffe76..f91f8071 100644 --- a/src/tests/api/internals/metadata/message_test.h +++ b/src/tests/api/internals/metadata/message_test.h @@ -348,10 +348,9 @@ TEST_P( EXPECT_TRUE(message2 >= message1); } -TEST_P(MessageTest, getContentShouldReturnADefaultContentObjectIfNoneExists) { +TEST_P(MessageTest, getContentShouldReturnANulloptIfNoneExists) { Message message; - EXPECT_EQ(MessageContent(), - message.GetContent(MessageContent::defaultLanguage)); + EXPECT_FALSE(message.GetContent(MessageContent::defaultLanguage).has_value()); } TEST_P( @@ -363,7 +362,7 @@ TEST_P( MessageContent("content2"), })); - EXPECT_EQ("content2", message.GetContent(french).GetText()); + EXPECT_EQ("content2", message.GetContent(french).value().GetText()); } TEST_P(MessageTest, getContentShouldSelectTheGivenLanguageStringIfItExists) { @@ -374,7 +373,7 @@ TEST_P(MessageTest, getContentShouldSelectTheGivenLanguageStringIfItExists) { MessageContent("content3", french), })); - EXPECT_EQ("content3", message.GetContent(french).GetText()); + EXPECT_EQ("content3", message.GetContent(french).value().GetText()); } TEST_P(MessageTest, getContentShouldSelectTheContentStringIfOnlyOneExists) { @@ -384,7 +383,7 @@ TEST_P(MessageTest, getContentShouldSelectTheContentStringIfOnlyOneExists) { })); EXPECT_EQ("content1", - message.GetContent(MessageContent::defaultLanguage).GetText()); + message.GetContent(MessageContent::defaultLanguage).value().GetText()); } TEST_P(MessageTest, toSimpleMessageShouldSelectTextAndLanguageUsingGetContent) { @@ -396,7 +395,7 @@ TEST_P(MessageTest, toSimpleMessageShouldSelectTextAndLanguageUsingGetContent) { }), "condition1"); - SimpleMessage simpleMessage = message.ToSimpleMessage(french); + SimpleMessage simpleMessage = message.ToSimpleMessage(french).value(); EXPECT_EQ(MessageType::warn, simpleMessage.type); EXPECT_EQ("content3", simpleMessage.text); diff --git a/src/tests/api/internals/metadata/plugin_cleaning_data_test.h b/src/tests/api/internals/metadata/plugin_cleaning_data_test.h index 0e5fe397..cc407484 100644 --- a/src/tests/api/internals/metadata/plugin_cleaning_data_test.h +++ b/src/tests/api/internals/metadata/plugin_cleaning_data_test.h @@ -322,16 +322,16 @@ TEST_P(PluginCleaningDataTest, chooseDetailShouldCreateADefaultContentObjectIfNoneExists) { PluginCleaningData dirtyInfo( 0xDEADBEEF, "cleaner", std::vector(), 2, 10, 30); - EXPECT_EQ(MessageContent(), - dirtyInfo.ChooseDetail(MessageContent::defaultLanguage)); + EXPECT_FALSE(dirtyInfo.ChooseDetail(MessageContent::defaultLanguage).has_value()); } TEST_P(PluginCleaningDataTest, chooseDetailShouldLeaveTheContentUnchangedIfOnlyOneStringExists) { PluginCleaningData dirtyInfo(0xDEADBEEF, "cleaner", info_, 2, 10, 30); - EXPECT_EQ(info_[0], dirtyInfo.ChooseDetail(french)); - EXPECT_EQ(info_[0], dirtyInfo.ChooseDetail(MessageContent::defaultLanguage)); + EXPECT_EQ(info_[0], dirtyInfo.ChooseDetail(french).value()); + EXPECT_EQ(info_[0], + dirtyInfo.ChooseDetail(MessageContent::defaultLanguage).value()); } TEST_P( @@ -344,7 +344,7 @@ TEST_P( }); PluginCleaningData dirtyInfo(0xDEADBEEF, "cleaner", info, 2, 10, 30); - EXPECT_EQ(content, dirtyInfo.ChooseDetail(french)); + EXPECT_EQ(content, dirtyInfo.ChooseDetail(french).value()); } TEST_P(PluginCleaningDataTest, @@ -357,7 +357,7 @@ TEST_P(PluginCleaningDataTest, }); PluginCleaningData dirtyInfo(0xDEADBEEF, "cleaner", info, 2, 10, 30); - EXPECT_EQ(frenchContent, dirtyInfo.ChooseDetail(french)); + EXPECT_EQ(frenchContent, dirtyInfo.ChooseDetail(french).value()); } TEST_P(PluginCleaningDataTest, emittingAsYamlShouldOutputAllNonZeroCounts) {