implement accent/diacritic ignore search#1724
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA custom SQLite function and supporting logic were added to enable case-insensitive and diacritic-insensitive string searching within the database context. The search logic and tests were updated to ensure that searching for characters like "A" will also match entries with "Ã", according to invariant culture rules. New extension methods for culture-aware string comparison were introduced. Changes
Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/FwLite/MiniLcm.Tests/SerializationTests.cs (1)
1-4: Verify necessity of unused using directives.The added using directives for
System.Globalization,System.Text, andMiniLcm.Culturedon't appear to be used in this file. Consider removing them unless they're needed for future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/FwLite/LcmCrdt/Data/CustomSqliteFunctionInterceptor.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs(1 hunks)backend/FwLite/LcmCrdt/SqlHelpers.cs(2 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(2 hunks)backend/FwLite/MiniLcm.Tests/SerializationTests.cs(1 hunks)backend/FwLite/MiniLcm/Culture/StringExtensions.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/FwLite/LcmCrdt/SqlHelpers.cs (4)
backend/FwLite/LcmCrdt/Json.cs (8)
Json(12-200)Sql(116-123)Sql(125-132)Sql(173-177)Sql(179-183)Expression(142-145)Expression(153-156)Expression(166-169)backend/FwLite/LcmCrdt/Data/Filtering.cs (1)
Expression(15-20)backend/FwLite/LcmCrdt/Data/CustomSqliteFunctionInterceptor.cs (1)
CustomSqliteFunctionInterceptor(9-33)backend/FwLite/MiniLcm/Culture/StringExtensions.cs (1)
Contains(7-10)
backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (1)
backend/FwLite/LcmCrdt/Data/CustomSqliteFunctionInterceptor.cs (1)
CustomSqliteFunctionInterceptor(9-33)
backend/FwLite/LcmCrdt/Data/CustomSqliteFunctionInterceptor.cs (1)
backend/FwLite/MiniLcm/Culture/StringExtensions.cs (1)
Contains(7-10)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (1)
18-18: LGTM! Custom SQLite function properly integrated.The addition of
CustomSqliteFunctionInterceptorcorrectly integrates the culture-aware string comparison functionality into the EF Core pipeline, enabling the custom "contains" function for LINQ-to-SQL translation.backend/FwLite/LcmCrdt/SqlHelpers.cs (2)
1-6: LGTM! Required using directives properly added.The new using directives correctly support the culture-aware string comparison functionality and SQLite function integration.
31-31: LGTM! Search logic updated to use culture-aware comparison.The change from direct
string.ContainstoContainsIgnoreCaseAccentscorrectly implements the accent/diacritic insensitive search functionality.backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
1-1: LGTM! Required using directive added.The
System.Textusing directive is correctly added to support Unicode normalization in the new tests.backend/FwLite/MiniLcm/Culture/StringExtensions.cs (2)
7-10: Well-implemented culture-aware Contains method.The implementation correctly uses
CompareInfo.IndexOfwith the appropriate return value check. This provides a solid foundation for culture-sensitive string searches.
12-18: Well-implemented culture-aware Equals method.The implementation correctly uses
CompareInfo.Compareand checks for zero return value to determine equality. The method signature and default parameter handling are appropriate.backend/FwLite/LcmCrdt/Data/CustomSqliteFunctionInterceptor.cs (2)
19-19: Good null safety handling.The null checks prevent potential exceptions when either parameter is null.
26-32: Proper async implementation.The async method correctly delegates to the synchronous version and returns a completed task, which is appropriate for this interceptor pattern.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Looks good! 💪 🎉
I pushed some tiny harmless changes.
…e in the search string, otherwise it does match
rip out diacritic check cache since it's actually slower to lookup the result
2 participants
closes #1718
Expected implementation:
Actual:
this also raised some normalization issues, so I'm going to get some input from Jason on that.