TwiN/go-away
1
0
Fork 0
Code Issues 8 Pull requests 2 Projects Releases 38 Packages Wiki Activity Actions

fix: Support cyrillic characters #27

Merged
martinhrvn merged 3 commits from issue_25 into master 2022-03-20 01:10:23 -04:00
Conversation 11 Commits 3 Files changed 2 +29 -11
martinhrvn commented 2022-03-15 15:53:27 -04:00 (Migrated from github.com)
Copy link

This fixes #25

Root cause of this issue is that strings.Index returns byte index. So I switched the calculation of the indexes to use rune index as well as taking the string up to specified index and from specified index. Mabye the censored string could be changed to []rune and instead of calling takeRunesFromIndex we could just update the censored inplace.

This fixes #25 Root cause of this issue is that `strings.Index` returns byte index. So I switched the calculation of the indexes to use rune index as well as taking the string up to specified index and from specified index. Mabye the censored string could be changed to `[]rune` and instead of calling `takeRunesFromIndex` we could just update the `censored` inplace.
TwiN (Migrated from github.com) reviewed 2022-03-16 17:48:41 -04:00
goaway.go
@ -166,1 +177,3 @@
currentIndex += foundIndex + len(word)
runeIndex := g.indexToRune(s, currentIndex+foundIndex)
originalIndexes = append(originalIndexes[:runeIndex], originalIndexes[runeIndex+len(word):]...)
currentIndex += foundIndex + len([]rune(word))
TwiN (Migrated from github.com) commented 2022-03-16 17:48:41 -04:00
Copy link

Is there any chance these three could be merged into one?
I haven't run a benchmark, but I feel this might add a noticeable overhead 🤔

Is there any chance these three could be merged into one? I haven't run a benchmark, but I feel this might add a noticeable overhead 🤔
TwiN (Migrated from github.com) reviewed 2022-03-16 17:51:00 -04:00
goaway.go
@ -166,1 +177,3 @@
currentIndex += foundIndex + len(word)
runeIndex := g.indexToRune(s, currentIndex+foundIndex)
originalIndexes = append(originalIndexes[:runeIndex], originalIndexes[runeIndex+len(word):]...)
currentIndex += foundIndex + len([]rune(word))
TwiN (Migrated from github.com) commented 2022-03-16 17:51:00 -04:00
Copy link

Actually, scratch that.
We may want to look into https://pkg.go.dev/unicode/utf8#DecodeRune instead:

DecodeRune unpacks the first UTF-8 encoding in p and returns the rune and its width in bytes. If p is empty it returns (RuneError, 0). Otherwise, if the encoding is invalid, it returns (RuneError, 1). Both are impossible results for correct, non-empty UTF-8.

Actually, scratch that. We may want to look into https://pkg.go.dev/unicode/utf8#DecodeRune instead: > DecodeRune unpacks the first UTF-8 encoding in p and returns the rune and its width in bytes. If p is empty it returns (RuneError, 0). Otherwise, if the encoding is invalid, it returns (RuneError, 1). Both are impossible results for correct, non-empty UTF-8.
martinhrvn (Migrated from github.com) reviewed 2022-03-17 05:08:49 -04:00
goaway.go
@ -166,1 +177,3 @@
currentIndex += foundIndex + len(word)
runeIndex := g.indexToRune(s, currentIndex+foundIndex)
originalIndexes = append(originalIndexes[:runeIndex], originalIndexes[runeIndex+len(word):]...)
currentIndex += foundIndex + len([]rune(word))
martinhrvn (Migrated from github.com) commented 2022-03-17 05:08:49 -04:00
Copy link

I added a change where we don't iterate several times, but actually just replace the rune with '*'

I added a change where we don't iterate several times, but actually just replace the rune with '*'
codecov-commenter commented 2022-03-17 19:06:27 -04:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #27 (cffad5e) into master (bd63346) will decrease coverage by 0.59%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##            master      #27      +/-   ##
===========================================
- Coverage   100.00%   99.40%   -0.60%     
===========================================
  Files            1        1              
  Lines          158      168      +10     
===========================================
+ Hits           158      167       +9     
- Misses           0        1       +1
Impacted Files Coverage Δ
goaway.go 99.40% <95.23%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd63346...cffad5e. Read the comment docs.

# [Codecov](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.) Report > Merging [#27](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.) (cffad5e) into [master](https://codecov.io/gh/TwiN/go-away/commit/bd63346fc7f0b3904d8211ed4215f3478e348fcd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.) (bd63346) will **decrease** coverage by `0.59%`. > The diff coverage is `95.23%`. ```diff @@ Coverage Diff @@ ## master #27 +/- ## =========================================== - Coverage 100.00% 99.40% -0.60% =========================================== Files 1 1 Lines 158 168 +10 =========================================== + Hits 158 167 +9 - Misses 0 1 +1 ``` | [Impacted Files](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.) | Coverage Δ | | |---|---|---| | [goaway.go](https://codecov.io/gh/TwiN/go-away/pull/27/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.#diff-Z29hd2F5Lmdv) | `99.40% <95.23%> (-0.60%)` | :arrow_down: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.). Last update [bd63346...cffad5e](https://codecov.io/gh/TwiN/go-away/pull/27?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chris+C.).
TwiN (Migrated from github.com) reviewed 2022-03-17 19:07:23 -04:00
goaway.go
@ -166,1 +177,3 @@
currentIndex += foundIndex + len(word)
runeIndex := g.indexToRune(s, currentIndex+foundIndex)
originalIndexes = append(originalIndexes[:runeIndex], originalIndexes[runeIndex+len(word):]...)
currentIndex += foundIndex + len([]rune(word))
TwiN (Migrated from github.com) commented 2022-03-17 19:07:23 -04:00
Copy link

Awesome!
Could you remove the takeRunesFromIndex and takeRunesToIndex functions then? 🙏

Awesome! Could you remove the `takeRunesFromIndex` and `takeRunesToIndex` functions then? 🙏
martinhrvn (Migrated from github.com) reviewed 2022-03-18 04:22:39 -04:00
goaway.go
@ -166,1 +177,3 @@
currentIndex += foundIndex + len(word)
runeIndex := g.indexToRune(s, currentIndex+foundIndex)
originalIndexes = append(originalIndexes[:runeIndex], originalIndexes[runeIndex+len(word):]...)
currentIndex += foundIndex + len([]rune(word))
martinhrvn (Migrated from github.com) commented 2022-03-18 04:22:39 -04:00
Copy link

yep, sorry, I missed those.

yep, sorry, I missed those.
TwiN (Migrated from github.com) approved these changes 2022-03-20 00:30:50 -04:00
omatija commented 2022-03-23 06:16:51 -04:00 (Migrated from github.com)
Copy link

Hello @TwiN I work with @martinhrvn and I have a question. Is there any future release planned? If yes can you share the date? We would like to use this fix in our code so is there any chance to release v1.6.1 ?
Thanks

Hello @TwiN I work with @martinhrvn and I have a question. Is there any future release planned? If yes can you share the date? We would like to use this fix in our code so is there any chance to release v1.6.1 ? Thanks
TwiN commented 2022-03-23 20:01:13 -04:00 (Migrated from github.com)
Copy link

@omatija I'll release it in a couple minutes; I've just been very busy these past weeks, but since you need it now, I'll accommodate you :)

@omatija I'll release it in a couple minutes; I've just been very busy these past weeks, but since you need it now, I'll accommodate you :)
TwiN commented 2022-03-23 20:17:19 -04:00 (Migrated from github.com)
Copy link

Thank you for your contribution!

Released in v1.6.1

Thank you for your contribution! Released in [v1.6.1](https://github.com/TwiN/go-away/releases/tag/v1.6.1)
omatija commented 2022-03-24 06:36:06 -04:00 (Migrated from github.com)
Copy link

Thank you very very much! :)

Thank you very very much! :)
Sign in to join this conversation.
Reviewers
No reviewers
TwiN
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TwiN/go-away!27
No description provided.