Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
danielcompton opened this issue Mar 13, 2023 · 5 comments · Fixed by #7205 or #7492
Closed
Labels
enhancement a request to improve CLI help wanted Contributions welcome

Comments

@danielcompton
Copy link

Describe the feature or problem you’d like to solve

I use the gh CLI to open PRs for review by coworkers. I like the feature where you can add reviewers from the CLI. I work with some coworkers whose names have diacritics, e.g. Miķelis, Klāvs, Dāvis.

When I try and search for their name in the CLI, I will type something like:

? Reviewers Mi  [Use arrows to move, space to select, <right> to all, <left> to none, type to filter]
> [ ]  testuser (Miķelis Test)

When I then type k, the person's name disappears from the list:

? Reviewers Mik  [Use arrows to move, space to select, <right> to all, <left> to none, type to filter]

Proposed solution

Allow searching for names with diacritics using "regular" characters. This will help users who have names that aren't "regular" names. I would expect when I type mik, this would match a user named Miķelis.

@danielcompton danielcompton added the enhancement a request to improve CLI label Mar 13, 2023
@cliAutomation cliAutomation added the needs-triage needs to be reviewed label Mar 13, 2023
@mislav
Copy link
Contributor

mislav commented Mar 13, 2023

Thanks for the feature request! I'm fully on board with this. 👍

Notes for would-be implementers

Wherever we instantiate a survey.Select or survey.MultiSelect, there is an implicit filtering functionality as provided by the Survey library. The default filter just matches on exact substrings and has an implementation similar or identical to:

&survey.Select{
	Filter: func(filterValue, optValue string, index int) bool {
		filterValue = strings.ToLower(filterValue)
		return strings.Contains(strings.ToLower(optValue), filterValue)
	},
}

To make it happen that a letter k when filtering would match ķ or that a would match ā, for example, Unicode runes in optValue should be transliterated to their Latin equivalents. Searching Go packages for "transliterate" and picking a trustworthy repo would be the way to go, in my opinion. Note that transliteration is a hard problem and that not all written scripts in the world might be covered by a single library.

Then, a Filter func with transliteration capabilities should be passed to places where we instantiate survey.Select or survey.MultiSelect within our codebase.

@danielcompton
Copy link
Author

danielcompton commented May 23, 2023

@benjlevesque thanks very much for working on this! I'm using gh 2.29.0 but I still seem to be getting the old behaviour here:

$ gh --version
gh version 2.29.0 (2023-05-10)
https://github.com/cli/cli/releases/tag/v2.29.0
gh pr create
Warning: 2 uncommitted changes

? Reviewers mi  [Use arrows to move, space to select, <right> to all, <left> to none, type to filter]
> [ ]  testuser (Miķelis test)

if I then type k, the remaining name disappears:

? Reviewers mik  [Use arrows to move, space to select, <right> to all, <left> to none, type to filter]

Does the LatinMatchingFilter need to be applied somewhere else as well?

@benjlevesque
Copy link
Contributor

@danielcompton I think you are right, the filter should probably be applied here:

Options: extraFieldsOptions,

I'm having trouble testing since I don't share a repo with someone with diacritics in their username, would you mind testing?

Prompt: &survey.MultiSelect{
  Message: "What would you like to add?",
  Options: extraFieldsOptions,
  Filter:  prompter.LatinMatchingFilter, // add this
},

@mislav
Copy link
Contributor

mislav commented May 24, 2023

@benjlevesque Feel free to open a PR, even if you are unable to test this. Yes, the LatinMatchingFilter should be applied to all survey prompts within the function that you've linked. I have asked for that here

@benjlevesque
Copy link
Contributor

Done here, @mislav 👍
I've found 4 places where I think it makes sense. Let me know if you think I missed others.

I was able to perform a test on labels with accents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a request to improve CLI help wanted Contributions welcome
Projects
None yet
5 participants
@mislav @danielcompton @benjlevesque @cliAutomation and others