Implement functions UCASE and LCASE #1060
Conversation
Missing: Unit tests and E2E tests, those will be added once the SUBSTR PR is merged.
src/util/StringUtils.h
Outdated
| // Get the uppercase value. For details see `getLowercaseUtf8` above | ||
| inline std::string getUppercaseUtf8(std::string_view s) { | ||
| std::string result; | ||
| icu::StringByteSink<std::string> sink(&result); | ||
| UErrorCode err = U_ZERO_ERROR; | ||
| icu::CaseMap::utf8ToUpper( | ||
| "", 0, icu::StringPiece{s.data(), static_cast<int32_t>(s.size())}, sink, | ||
| nullptr, err); | ||
| if (U_FAILURE(err)) { | ||
| throw std::runtime_error(u_errorName(err)); | ||
| } | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
Given the almost identical code, how about a helper function utf8StringTransform, which takes a std::string_view and the transformation function as arguments?
Also, it looks like it would be more consistent to name the two functions based on this helper function utf8StringToLower (instead of getLowercaseUtf8) and utf8StringToUpper (instead of getUppercaseUtf8).
| if (!input.has_value()) { | ||
| return Id::makeUndefined(); | ||
| } else { |
There was a problem hiding this comment.
A cascaded question: When exactly is the argument std.:nullopt, when the argument given in the SPARQL query was not actually a string? If yes, is the result of a built-in SPARQL function taking one or several string as argument always UNDEF when one of these arguments is not a string? If yes, should this be handled in the functions or outside? I am asking because for the numeric functions the return Id::makeUndefined happens elsewhere.
There was a problem hiding this comment.
This is actually a good idea.
For the Numeric functions we have a wrapper that handles the "invalid input" -> expression error -> UNDEF result case automagically,
But we could extend this to all kinds of expressions, s.t. for example the string function implementations only need to handle a string instead of an std::optional<std::string>.
I will think of something and integrate it into one of the next PRs.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1060 +/- ##
==========================================
- Coverage 78.29% 78.24% -0.06%
==========================================
Files 276 276
Lines 25703 25740 +37
Branches 3144 3149 +5
==========================================
+ Hits 20123 20139 +16
- Misses 4378 4395 +17
- Partials 1202 1206 +4
☔ View full report in Codecov by Sentry. |
|
Kudos, SonarCloud Quality Gate passed!
|
hannahbast
changed the title
Missing: Unit tests and E2E tests, those will be added once the SUBSTR PR is merged.