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

Fix typo in misc tests #1270

Merged
merged 1 commit into from
Jun 4, 2023
Merged

Fix typo in misc tests #1270

merged 1 commit into from
Jun 4, 2023

Conversation

joelim-work
Copy link
Collaborator

@joelim-work joelim-work commented May 28, 2023

If the unit test fails, it prints out the input twice instead of the expected value.


BTW I was having a look at the runeSliceWidthRange function because @raslop was working on this in #1029. Unfortunately the current implementation in the master branch does appear to be slightly buggy if you consider the following test cases:

{[]rune{'世', '界', '世', '界'}, 3, 3, []rune{}},
{[]rune{'世', '界', '世', '界'}, 4, 7, []rune{'世'}},
{[]rune{'世', '界', '世', '界'}, 4, 8, []rune{'世', '界'}},

In particular the current implementation panics when the beginning and end indexes are the same and happen to be in the middle of a rune, and also the case where the end occurs after the start of the last rune isn't handled properly. The following is a more robust implementation that does handle the above cases in addition to the existing cases:

func runeSliceWidthRange(rs []rune, beg, end int) []rune {
        if beg == end {
                return []rune{}
        }

        curr := 0
        b := 0
        foundb := false
        for i, r := range rs {
                w := runewidth.RuneWidth(r)
                if curr >= beg && !foundb {
                        b = i
                        foundb = true
                }
                if curr == end || curr+w > end {
                        return rs[b:i]
                }
                curr += w
        }

        return rs[b:]
}

That being said, runeSliceWidthRange is currently called in only a few places when truncating long filenames, and for these cases the filename input is sufficiently long so these edge cases don't matter, so I can understand if you don't want to make any changes to the existing code.

Anyway the suggestion about runeSliceWidthRange is a separate thing, and can be addressed later if at all. At the very least the change for correcting the test case error reporting should be merged.

@raslop
Copy link
Contributor

raslop commented May 28, 2023

The typo-fix commit here is exactly the same that I stumbled upon earlier today and also put into #1029.
The suggestion here is a better version of my try to fix runeSliceWidthRange() in #1029 (which works fine there btw.). I would like to see this suggestion merged too. Once this is done, I will then try to get #1029 (with only the feature commit left) merged.

Copy link
Collaborator

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

The fix looks good, thanks!

@joelim-work
Copy link
Collaborator Author

The suggestion here is a better version of my try to fix runeSliceWidthRange() in #1029 (which works fine there btw.). I would like to see this suggestion merged too.

I have submitted this change, see #1273

@gokcehan gokcehan merged commit c3d4412 into gokcehan:master Jun 4, 2023
@joelim-work joelim-work deleted the fix-test branch June 4, 2023 23:14
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