Refactor the parsing of the text index builder #1695
Conversation
|
One question I have is that I didn't find a solution to convert absl::StrSplit to a std::range or std::view and therefore resulted to using another cppcoro generator. I've seen the idea to avoid these generators but am I right that it is only possible to use these new Iterators through creating classes that implement them? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1695 +/- ##
=======================================
Coverage 89.86% 89.86%
=======================================
Files 389 390 +1
Lines 37308 37339 +31
Branches 4204 4205 +1
=======================================
+ Hits 33527 33556 +29
+ Misses 2485 2483 -2
- Partials 1296 1300 +4 ☔ View full report in Codecov by Sentry. |
This reverts commit c365935.
…sts in WordsAndDocsFileParserTest.cpp. Renamed methods in WordsAndDocsFileLineCreator.h to reduce ambiguity. Incorporated requested small changes of PR.
Signed-off-by: Johannes Kalmbach <[email protected]>
| ASSERT_EQ(std::get<0>(testLine), std::get<0>(expectedResult.at(i))); | ||
| ASSERT_EQ(std::get<1>(testLine), std::get<1>(expectedResult.at(i))); | ||
| ASSERT_EQ(std::get<2>(testLine), std::get<2>(expectedResult.at(i))); | ||
| ASSERT_EQ(std::get<3>(testLine), std::get<3>(expectedResult.at(i))); |
There was a problem hiding this comment.
That is much better with the helper functions (There are even cleaner ways with better error messages in GoogleTest, but this refactoring is nice because now all improviements can be applied locally!
…d be outsourced in further refactorings
|
One possible solution to the current coverage problem is to start a file IndexImplHelpers.h and a corresponding cpp to outsource the helper methods and test them seperately. This would leed to even more references being passed to the functions. Currently I am unsure whether to do this or not. Also maybe there is another way to reduce the nesting at the positions where the helper functions are now at play as solution. |
src/index/IndexImpl.Text.cpp
Outdated
| @@ -53,8 +53,7 @@ cppcoro::generator<WordsFileLine> IndexImpl::wordsInTextRecords( | |||
| std::string_view textView = text; | |||
| textView = textView.substr(0, textView.rfind('"')); | |||
| textView.remove_prefix(1); | |||
| auto normalizedWords = tokenizeAndNormalizeText(textView, localeManager); | |||
| for (auto word : normalizedWords) { | |||
| for (auto word : tokenizeAndNormalizeText(textView, localeManager)) { | |||
| WordsFileLine wordLine{word, false, contextId, 1}; | |||
There was a problem hiding this comment.
Do we benefit from a std::move(word) here?
Conformance check passed ✅No test result changes. |
|
joka921
changed the title
Split up large functions, modernize code, choose better names and add some documentation