Skip to content

Conversation

@h3x0c4t
Copy link

@h3x0c4t h3x0c4t commented Apr 3, 2025

Hi!
I use readline and console in my project and I needed Cyrillic support, so I added the ability to enter unicode characters :)

image

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I made a small feedback as I feel like the code could be simpler

Comment on lines 131 to 166
// PeekChar returns the first character in the stack, without removing it.
func PeekChar(keys *Keys) (key string, empty bool) {
bufStr := []rune(string(keys.buf))
switch {
case len(bufStr) > 0:
key = string(bufStr[0])
default:
return "", true
}

return key, false
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this?

Suggested change
// PeekChar returns the first character in the stack, without removing it.
func PeekChar(keys *Keys) (key string, empty bool) {
bufStr := []rune(string(keys.buf))
switch {
case len(bufStr) > 0:
key = string(bufStr[0])
default:
return "", true
}
return key, false
}
// PeekChar returns the first character in the stack, without removing it.
func PeekChar(keys *Keys) (key string, empty bool) {
if len(keys.buf) == 0 {
return "", true
}
return []rune(string(keys.buf))[0], false
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have made some changes to this function.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yours is simpler to read, yes

@lowelli
Copy link

lowelli commented Apr 4, 2025

Awecome!

Can we expect this PR to be accepted and a new version of readline package created?

Thx in advance!

@maxlandon
Copy link
Member

Hello @h3x0c4t, @ccoVeille,

Thanks a lot for your PR.
I will review it this week-end, and merge accordingly.
(I have to check how that fits with the various .inputrc options for character handling).

Thanks again.

@maxlandon
Copy link
Member

Hello @h3x0c4t and @ccoVeille,

I wanted to know if you found any functionality bug related to this change ?
Do most keys/actions perform as before ?
Is terminal display unaffected ?
Anything else ?

Thanks in advance

@codecov
Copy link

codecov bot commented May 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 67 lines in your changes missing coverage. Please review.

Project coverage is 34.31%. Comparing base (51a4df8) to head (1a194fa).
Report is 56 commits behind head on master.

Files with missing lines Patch % Lines
internal/core/keys.go 0.00% 28 Missing ⚠️
internal/keymap/dispatch.go 0.00% 26 Missing ⚠️
internal/display/highlight.go 0.00% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #81       +/-   ##
===========================================
- Coverage   89.30%   34.31%   -55.00%     
===========================================
  Files          10       57       +47     
  Lines        4739    12620     +7881     
===========================================
+ Hits         4232     4330       +98     
- Misses        470     8252     +7782     
- Partials       37       38        +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

unicode support

simplified

Adapt key-matching code to support Unicode

Fix comment typo
@maxlandon maxlandon merged commit b9747bd into reeflective:master May 4, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants