-
Notifications
You must be signed in to change notification settings - Fork 17
Unicode support #81
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
Unicode support #81
Conversation
ccoVeille
left a comment
There was a problem hiding this 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
internal/core/keys.go
Outdated
| // 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 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
| // 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 | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Awecome! Can we expect this PR to be accepted and a new version of readline package created? Thx in advance! |
|
Hello @h3x0c4t, @ccoVeille, Thanks a lot for your PR. Thanks again. |
|
Hello @h3x0c4t and @ccoVeille, I wanted to know if you found any functionality bug related to this change ? Thanks in advance |
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
unicode support simplified Adapt key-matching code to support Unicode Fix comment typo
Hi!
I use readline and console in my project and I needed Cyrillic support, so I added the ability to enter unicode characters :)