Diacritics substitution in prompt by benjlevesque · Pull Request #7205 · cli/cli

benjlevesque · 2023-03-21T00:27:50Z

I probably missed places to add the filter to, I would like to get some feedback before moving forward.
@mislav thanks for your tips. The standard transform library seems to do the trick...

mislav

Nice, thanks for kicking this off!

Note that the Prompter is not the only place where we instantiate Select/MultiSelect. For example, surveys containing names of people as options (e.g. assignees, reviewers) are mainly made in the MetadataSurvey method. There are other Selects in the codebase, but they don't need to be updated with the new Filter functionality because their options will typically not contain names of people.

internal/text/text.go

mislav · 2023-03-22T21:35:32Z

internal/text/text_test.go

+		{"e", "e"},
+
+		// unsupported characters remain untouched
+		{"ؤ", "و"},


I'm seeing that a Unicode codepoint U+624 got converted into U+648. Shouldn't this test verify that unsupported characters remain untouched?

Also, could there be more assertions about Unicode codepoints remaining untouched, e.g. scripts like Cyrillic or some Asian scripts?

oops, yes that's wrong, good catch.
I'm starting to doubt the relevance of this "unsupported characters" test scenario, I actually can't find a case where it does not work so far.

internal/text/text_test.go

mislav · 2023-03-22T21:39:10Z

internal/prompter/prompter.go

 }

+func Filter(filter, value string, index int) bool {
+	filter = text.RemoveDiacritics(strings.ToLower(filter))


Should we be removing diacritics from the filter string? If a user has entered a filter string with diacritics, e.g. ć, that is a good indicator that they meant to specifically search for that character and not for c. Maybe we should leave filter unchanged and just "normalize" the value string?

for this one, I went with the same behaviour as ToLower, which is by default applied to both filter and value.

I think it's valid also for diacritics since you could type a name thinking the value contains diacritics whereas it does not?

No strong opinion, if you want to change this I can 👍

internal/prompter/prompter.go

mislav

Looks great; thank you for the updates! One tiny request remaining

mislav · 2023-04-03T12:08:46Z

internal/prompter/prompter.go

+// LatinMatchingFilter returns whether the value matches the input filter.
+// The strings are compared normalized in case and diacritics.
+func LatinMatchingFilter(filter, value string, index int) bool {
+	filter = strings.ToLower(text.RemoveDiacritics(filter))


Let's keep the diacritics in the original filter string, and just ToLower it. If someone typed in ø wanting to search for that character specifically, we'd be doing them a disservice by matching that against a literal o.

Hey @mislav, this is done in 91a828a, sorry for the delay.
I had to modify this function to compare the strings twice, first with diacritics, then with no diacritics in the value

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [aquaproj/aqua-registry](https://togithub.com/aquaproj/aqua-registry) | minor | `v3.158.0` -> `v3.159.0` | | [cli/cli](https://togithub.com/cli/cli) | minor | `v2.27.0` -> `v2.28.0` | | [weaveworks/eksctl](https://togithub.com/weaveworks/eksctl) | minor | `v0.138.0` -> `v0.139.0` | --- ### Release Notes <details> <summary>aquaproj/aqua-registry</summary> ### [`v3.159.0`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.159.0) [Compare Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.1...v3.159.0) [Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.159.0) | [Pull Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.159.0) | aquaproj/aqua-registry@v3.158.0...v3.159.0 #### 🎉 New Packages [#11807](https://togithub.com/aquaproj/aqua-registry/issues/11807) [kubecfg/kubecfg](https://togithub.com/kubecfg/kubecfg): A tool for managing complex enterprise Kubernetes environments as code [#11808](https://togithub.com/aquaproj/aqua-registry/issues/11808) [loov/goda](https://togithub.com/loov/goda): Go Dependency Analysis toolkit #### Fixes [#11806](https://togithub.com/aquaproj/aqua-registry/issues/11806) solidiquis/erdtree: Follow up changes of erdtree v2.0.0 https://github.com/solidiquis/erdtree/releases/tag/v2.0.0 > Perhaps the most important change to note is that the compiled binary has been renamed from et to erd in order to address the following issue > regarding name collisions with other programs > > - [https://github.com/solidiquis/erdtree/issues/23](https://togithub.com/solidiquis/erdtree/issues/23) ### [`v3.158.1`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.158.1) [Compare Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.0...v3.158.1) [Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.158.1) | [Pull Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.158.1) | aquaproj/aqua-registry@v3.158.0...v3.158.1 #### Fixes [#11790](https://togithub.com/aquaproj/aqua-registry/issues/11790) Follow up changes of cli/cli v2.28.0 [@kyontan](https://togithub.com/kyontan) GitHub's CLI (cli/cli) changed format for macOS to zip (from tar.gz) since v2.28.0 See https://github.com/cli/cli/releases/tag/v2.28.0 for details. </details> <details> <summary>cli/cli</summary> ### [`v2.28.0`](https://togithub.com/cli/cli/releases/tag/v2.28.0): GitHub CLI 2.28.0 [Compare Source](https://togithub.com/cli/cli/compare/v2.27.0...v2.28.0) #### What's New - macOS binaries are now signed and notarized - ⚠️ macOS archives attached to our releases are no longer `.tar.gz`, but `.zip` instead. This is because `.tar.gz` archives cannot be notarized. - The `checksums.txt` file attached to every release now includes the checksum of the Windows MSI installer too. - macOS and Windows binaries are now compiled from their respective platforms and have `cgo` enabled. This might help resolve respecting system proxy settings and avoid related networking issues. - `issue edit`: edit multiple issues at the same time by [@heaths](https://togithub.com/heaths) in [https://github.com/cli/cli/pull/7259](https://togithub.com/cli/cli/pull/7259) - Add `gh org list` by [@joshkraft](https://togithub.com/joshkraft) in [https://github.com/cli/cli/pull/7257](https://togithub.com/cli/cli/pull/7257) - `ssh-key`: add ability to manage signing keys by [@kousikmitra](https://togithub.com/kousikmitra) in [https://github.com/cli/cli/pull/7270](https://togithub.com/cli/cli/pull/7270) - `search`: enable owner flag to take multiple values by [@kousikmitra](https://togithub.com/kousikmitra) in [https://github.com/cli/cli/pull/7305](https://togithub.com/cli/cli/pull/7305) - `codespace`: add `--web` flag for `list` & `create` commands by [@doaortu](https://togithub.com/doaortu) in [https://github.com/cli/cli/pull/7288](https://togithub.com/cli/cli/pull/7288) - Our Debian & RPM packages now ship with shell completion scripts by [@Xerkus](https://togithub.com/Xerkus) in [https://github.com/cli/cli/pull/7293](https://togithub.com/cli/cli/pull/7293) - `run list`: add `--event` and `--created` filters by [@cawfeecake](https://togithub.com/cawfeecake) in [https://github.com/cli/cli/pull/7363](https://togithub.com/cli/cli/pull/7363) [https://github.com/cli/cli/pull/7352](https://togithub.com/cli/cli/pull/7352) - `repo`: add `visibility` JSON field by [@yeikel](https://togithub.com/yeikel) in [https://github.com/cli/cli/pull/7337](https://togithub.com/cli/cli/pull/7337) #### What's Changed - Fix typo in `cs stop` command: `Stoppping` -> `Stopping` by [@FalseDev](https://togithub.com/FalseDev) in [https://github.com/cli/cli/pull/7318](https://togithub.com/cli/cli/pull/7318) - Update go-gh to v2 by [@samcoe](https://togithub.com/samcoe) in [https://github.com/cli/cli/pull/7299](https://togithub.com/cli/cli/pull/7299) - `auth login`: normalize host name by [@tuananhlai](https://togithub.com/tuananhlai) in [https://github.com/cli/cli/pull/6999](https://togithub.com/cli/cli/pull/6999) - build(deps): bump github.com/cenkalti/backoff/v4 from 4.2.0 to 4.2.1 by [@dependabot](https://togithub.com/dependabot) in [https://github.com/cli/cli/pull/7323](https://togithub.com/cli/cli/pull/7323) - Clarify how SSH keys are selected for `gh codespace ssh` by [@jkeech](https://togithub.com/jkeech) in [https://github.com/cli/cli/pull/7325](https://togithub.com/cli/cli/pull/7325) - Initialize deployment.yml workflow file by [@mislav](https://togithub.com/mislav) in [https://github.com/cli/cli/pull/7328](https://togithub.com/cli/cli/pull/7328) - Fix `gh cs ports` requiring `sudo` for privileged port numbers by [@cmbrose](https://togithub.com/cmbrose) in [https://github.com/cli/cli/pull/7326](https://togithub.com/cli/cli/pull/7326) - Correct some typos by [@goggle](https://togithub.com/goggle) in [https://github.com/cli/cli/pull/7342](https://togithub.com/cli/cli/pull/7342) - Diacritics substitution in prompt by [@benjlevesque](https://togithub.com/benjlevesque) in [https://github.com/cli/cli/pull/7205](https://togithub.com/cli/cli/pull/7205) - gh: move `CODEOWNERS` inside the `.github/` dir by [@SauravMaheshkar](https://togithub.com/SauravMaheshkar) in [https://github.com/cli/cli/pull/7366](https://togithub.com/cli/cli/pull/7366) - Pretty-print gh api output when using --jq by [@mjpieters](https://togithub.com/mjpieters) in [https://github.com/cli/cli/pull/7236](https://togithub.com/cli/cli/pull/7236) #### New Contributors - [@joshkraft](https://togithub.com/joshkraft) made their first contribution in [https://github.com/cli/cli/pull/7257](https://togithub.com/cli/cli/pull/7257) - [@kousikmitra](https://togithub.com/kousikmitra) made their first contribution in [https://github.com/cli/cli/pull/7270](https://togithub.com/cli/cli/pull/7270) - [@doaortu](https://togithub.com/doaortu) made their first contribution in [https://github.com/cli/cli/pull/7288](https://togithub.com/cli/cli/pull/7288) - [@FalseDev](https://togithub.com/FalseDev) made their first contribution in [https://github.com/cli/cli/pull/7318](https://togithub.com/cli/cli/pull/7318) - [@tuananhlai](https://togithub.com/tuananhlai) made their first contribution in [https://github.com/cli/cli/pull/6999](https://togithub.com/cli/cli/pull/6999) - [@goggle](https://togithub.com/goggle) made their first contribution in [https://github.com/cli/cli/pull/7342](https://togithub.com/cli/cli/pull/7342) - [@Xerkus](https://togithub.com/Xerkus) made their first contribution in [https://github.com/cli/cli/pull/7293](https://togithub.com/cli/cli/pull/7293) - [@cawfeecake](https://togithub.com/cawfeecake) made their first contribution in [https://github.com/cli/cli/pull/7363](https://togithub.com/cli/cli/pull/7363) - [@yeikel](https://togithub.com/yeikel) made their first contribution in [https://github.com/cli/cli/pull/7337](https://togithub.com/cli/cli/pull/7337) - [@SauravMaheshkar](https://togithub.com/SauravMaheshkar) made their first contribution in [https://github.com/cli/cli/pull/7366](https://togithub.com/cli/cli/pull/7366) - [@mjpieters](https://togithub.com/mjpieters) made their first contribution in [https://github.com/cli/cli/pull/7236](https://togithub.com/cli/cli/pull/7236) **Full Changelog**: cli/cli@v2.27.0...v2.28.0 </details> <details> <summary>weaveworks/eksctl</summary> ### [`v0.139.0`](https://togithub.com/weaveworks/eksctl/releases/tag/v0.139.0): eksctl 0.139.0 (permalink) [Compare Source](https://togithub.com/weaveworks/eksctl/compare/0.138.0...0.139.0) ### Release v0.139.0 #### 🚀 Features - Security Policy for eksctl project ([#6541](https://togithub.com/weaveworks/eksctl/issues/6541)) #### 🐛 Bug Fixes - Fix flux version validation ([#6530](https://togithub.com/weaveworks/eksctl/issues/6530)) #### 📝 Documentation - Fix empty info block on Default Addon Upgrades page ([#6524](https://togithub.com/weaveworks/eksctl/issues/6524)) - Update installation instructions ([#6376](https://togithub.com/weaveworks/eksctl/issues/6376)) - AWS Private link support for fully private cluster ([#6408](https://togithub.com/weaveworks/eksctl/issues/6408)) #### Acknowledgments Weaveworks would like to sincerely thank: [@thezanke](https://togithub.com/thezanke), and [@yuxiang-zhang](https://togithub.com/yuxiang-zhang) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/scottames/dots).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

benjlevesque requested a review from a team as a code owner March 21, 2023 00:27

benjlevesque requested review from samcoe and removed request for a team March 21, 2023 00:27

cliAutomation added the external pull request originating outside of the CLI core team label Mar 21, 2023

mislav suggested changes Mar 22, 2023

View reviewed changes

samcoe assigned mislav Mar 22, 2023

mislav mentioned this pull request Mar 23, 2023

Updated to sanitize asset filename prior to uploading. #7201

Closed

benjlevesque added 7 commits April 1, 2023 18:49

Diacritics substitution in prompt

febea2d

apply ToLower on the result of RemoveDiacritics

cd7f354

rename Filter to LatinMatchingFilter

2c3ee02

add test cases

0f0775c

add comments

a070119

simplify line

3c14def

add tests

f6a252a

benjlevesque force-pushed the issue-7142 branch from 40bcaae to f6a252a Compare April 1, 2023 17:17

add example

8ee528c

benjlevesque requested a review from mislav April 1, 2023 17:19

mislav suggested changes Apr 3, 2023

View reviewed changes

keep filter's diacritics

91a828a

mislav approved these changes Apr 21, 2023

View reviewed changes

mislav enabled auto-merge (squash) April 21, 2023 14:08

mislav merged commit 7cfbf47 into cli:trunk Apr 21, 2023

jtpetty pushed a commit that referenced this pull request May 22, 2023

Diacritics substitution in prompt (#7205)

8946d54

mislav mentioned this pull request May 24, 2023

Feature request: Allow searching for names with diacritics using "regular" characters #7142

Closed

benjlevesque deleted the issue-7142 branch May 26, 2023 12:51

benjlevesque mentioned this pull request May 26, 2023

fix: apply latin matching filter to more fields #7492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diacritics substitution in prompt#7205

Diacritics substitution in prompt#7205
mislav merged 9 commits intocli:trunkfrom
benjlevesque:issue-7142

benjlevesque commented Mar 21, 2023 •

edited

Loading

Uh oh!

mislav left a comment

Uh oh!

Uh oh!

mislav Mar 22, 2023

Uh oh!

benjlevesque Apr 1, 2023

Uh oh!

Uh oh!

mislav Mar 22, 2023

Uh oh!

benjlevesque Apr 1, 2023

Uh oh!

Uh oh!

Uh oh!

mislav left a comment

Uh oh!

mislav Apr 3, 2023

Uh oh!

benjlevesque Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

benjlevesque commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mislav Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

benjlevesque Apr 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mislav Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

benjlevesque Apr 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

mislav Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

benjlevesque Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjlevesque commented Mar 21, 2023 •

edited

Loading