Ignore accents when doing a bookmark search #3255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jotaemepereira
requested review from
a team and
federicocappelli
and removed request for
a team
federicocappelli
requested changes
Sep 12, 2024
There was a problem hiding this comment.
A few comments, mostly around naming, documentation and unit tests @mainactor use
| @@ -85,6 +85,24 @@ extension String { | |||
| self.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | |||
| } | |||
|
|
|||
| var cleanStringForBookmarkSearch: String { | |||
There was a problem hiding this comment.
Very picky notes:
- In general, functions that get self and return a modified version of self are named like
replacingOccurrences(of:...), this could bereplacing....orcleaning... - This is a function in a shared framework but is very specific and undocumented, I see 2 options here:
a. Use a more generic name and add documentation explaining the input, the expected output, the use cases and examples.
b. Leave it as it is but move it from the shared extension to a private, bookmarks only, extension
There was a problem hiding this comment.
I will move the extension to where is used and make it private. This is not going to be used elsewhere so it makes sense to remove it from here 👍🏼 .
About the name, I’m renaming it to: cleaningStringForBookmarkSearch. I will also add some documentation to it.
| .lowercased() | ||
| } | ||
|
|
||
| private func removeAccents() -> String { |
There was a problem hiding this comment.
Same as above, very generic name and no documentation.
The above naming convention comment is valid for this too.
There was a problem hiding this comment.
@federicocappelli What’s generic about the name 🤔 ? Removing accents seems clear to me, the function removes any accent from the string. What other naming suggestions do you have in mind?
I will add some documentation so it’s clearer.
There was a problem hiding this comment.
sorry, with generic name I meant that it doesn't follow the usual naming convention replacing.... or cleaning... etc
jotaemepereira
deleted the
juan/ignore-accents-when-searching-bookmarks
branch
samsymons
added a commit
that referenced
this pull request
Sep 12, 2024
# By Dominik Kapusta (4) and others # Via Dominik Kapusta (4) and GitHub (2) * main: Support dragging strings to new tab button (#3228) BSK - Add feature flag for SKAN API (#3265) Ignore accents when doing a bookmark search (#3255) Update UI tests workflows to always compile the app on the supported version of Xcode (#3262) Bump C.S.S (via BSK) (#3244) Tab previews presentation implementation adjusted for the latest MouseOverView logic (#3260) Bump version to 1.106.0 (260) Default to appstore signing for fastlane match Clean up secrets in GHA workflows Update Onboarding gradients (#3248) Wait for UserScripts to be loaded for email protection (#3252) Fix UI tests failing in the CI (#3254) Fix WKDownloadMock mock (#3258) VPN Proxy stop and pixel improvements (#3239) Use fastlane match for code signing DMG builds (#3246) Implement new VPN site troubleshooting report alert (#3224) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons
added a commit
that referenced
this pull request
Sep 13, 2024
# By Dominik Kapusta (7) and others # Via Dominik Kapusta (6) and GitHub (4) * main: (37 commits) Add visted links support (#3261) Bump version to 1.106.0 (261) Update Gemfile.lock Remove migration file (#3270) Use new AsanaCreateActionItem action (#3264) Update survey builder OS version (#3247) Support dragging strings to new tab button (#3228) BSK - Add feature flag for SKAN API (#3265) Ignore accents when doing a bookmark search (#3255) Update UI tests workflows to always compile the app on the supported version of Xcode (#3262) Bump C.S.S (via BSK) (#3244) Tab previews presentation implementation adjusted for the latest MouseOverView logic (#3260) Bump version to 1.106.0 (260) Default to appstore signing for fastlane match Clean up secrets in GHA workflows Update Onboarding gradients (#3248) Wait for UserScripts to be loaded for email protection (#3252) Fix UI tests failing in the CI (#3254) Fix WKDownloadMock mock (#3258) VPN Proxy stop and pixel improvements (#3239) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/1203972458584429/1208262956542583/f
Tech Design URL:
CC:
Description
Acceptance criteria
AC#1
Given a user with a bookmark with an accent, for example
café, when searching forcafewe should return the bookmark with the accent.AC#2
Given a user with a bookmark without an accent, for example
cafe, when searching forcaféwe should return the bookmark without the accent, ignoring the accent on the search query.AC#3
Given a user with a bookmark with symbols, for example, Site • Login , when searching for site login then we should return the bookmark with the symbol
AC#4
Given a user with a bookmark without symbols, for example, Site Login , when searching for site • login then we should return the bookmark without the symbol ignoring the symbol in the search query.
Steps to test this PR
Accents
cafécafe(without the accent)cafe, and search forcafé, the accent should be ignored and the result should be returned in the search.Symbols
Site • Loginsite login(without the symbol)Definition of Done:
—
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation