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

trim prefix #65

Merged
merged 7 commits into from
Sep 20, 2022
Merged

trim prefix #65

merged 7 commits into from
Sep 20, 2022

Conversation

tty2
Copy link
Contributor

@tty2 tty2 commented Sep 15, 2022

trim prefix method:

  • cuts n cells from the beginning of the string
  • adds prefix to string if set

@mattn
Copy link
Owner

mattn commented Sep 15, 2022

I don't understand why it's xxx... instead of ...xxx

@tty2
Copy link
Contributor Author

tty2 commented Sep 15, 2022

Cause it's a tail.
To be honest I'm not really sure if tail is necessary in this function. I just wanted to keep similarity with Truncate. But in my case it's not necessary.
Do you mean that it's more obvious to set tail as a prefix? It's easy to implement as well.

@tty2
Copy link
Contributor Author

tty2 commented Sep 15, 2022

Yes. I took a look at it more careful. You are right. It's stupid of me. Let me refactor this.

@tty2
Copy link
Contributor Author

tty2 commented Sep 15, 2022

Done!
Check it again please.

@mattn
Copy link
Owner

mattn commented Sep 16, 2022

Thanks. I'll look this in later.

@@ -380,6 +380,76 @@ func TestTruncateNoNeeded(t *testing.T) {
}
}

func Test_TrimPrefix(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

Remove _

@mattn
Copy link
Owner

mattn commented Sep 16, 2022

Thanks your contribution. I prefer the tests should be table-driven tests. See other tests.

@tty2
Copy link
Contributor Author

tty2 commented Sep 16, 2022

Sure. I should have written them similar.
Changed.

@mattn
Copy link
Owner

mattn commented Sep 16, 2022

Hmm, I'm still wondering what is this use-case. Could you please explain how this can be useful?

@tty2
Copy link
Contributor Author

tty2 commented Sep 16, 2022

:)
Check this PR.
It's one of the way how to implement horizontal scroll.
And the final goal is my personal tui app :) the problem is mentioned in this issue

@tty2
Copy link
Contributor Author

tty2 commented Sep 16, 2022

So instead of using getStringWithIndent function that is written by me in the PR I mentioned above, I'm gonna use this TrimPrefix function that will take care of cell not runes.

@mattn
Copy link
Owner

mattn commented Sep 16, 2022

So TrimPrefix("あいうえお", 5, "") must be " えお"

あいうえお
Aあいうえお
あいうえお
Aあいうえお

Result of trimming 5 cells should be

 えお
うえお
 えお
うえお

@tty2
Copy link
Contributor Author

tty2 commented Sep 16, 2022

So TrimPrefix("あいうえお", 5, "") must be " えお"

あいうえお
Aあいうえお
あいうえお
Aあいうえお

Result of trimming 5 cells should be

 えお
うえお
 えお
うえお

Nope. TrimPrefix("あいうえお", 5, "") returns "うえお"

And the result of TrimPrefix for this:

あいうえお
Aあいうえお
あいうえお
Aあいうえお

will be the same. I mean "うえお". Because part that we cut is not bigger than 5.

あい:	6 bytes, 2 runes, 4 cells, 2 grapheme clusters
Aあい:	7 bytes, 3 runes, 5 cells, 3 grapheme clusters
あい:	6 bytes, 2 runes, 4 cells, 2 grapheme clusters
Aあい:	7 bytes, 3 runes, 5 cells, 3 grapheme clusters

@tty2
Copy link
Contributor Author

tty2 commented Sep 16, 2022

Or...
You probably wanted to say that there is a way how to improve the logic? Aren't you?
Add one empty cell if there is a difference between w and pos == 1.

If so, on line 235 inside if condition we can change logic to:

...
	if width+chWidth > w {
		if width < w {
			_, pos = g.Positions()
			prefix += strings.Repeat(" ", w-width)
		} else {
			pos, _ = g.Positions()
		}

		break
	}
...

@tty2
Copy link
Contributor Author

tty2 commented Sep 17, 2022

I've pushed the implementation with logic from above comment.
And I decided to change the function name to TruncateLeft. It's more obvious, especially because there is a Truncate function.
Another reason is that there is a function in standard library strings.TrimPrefix that cut string relying on prefix (if it exists). For users this name similarity can play a bad game.
WDYT?

@mattn
Copy link
Owner

mattn commented Sep 19, 2022

Look good to me.

@mattn mattn merged commit 2c6a438 into mattn:master Sep 20, 2022
@mattn
Copy link
Owner

mattn commented Sep 20, 2022

Thank you

@tty2
Copy link
Contributor Author

tty2 commented Sep 20, 2022

Thank you too

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.

2 participants