text: add correct handling of hyperlink escape sequences (https://gis… #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 4268303129
💛 - Coveralls |
jedib0t
requested changes
Feb 21, 2023
Makefile
Outdated
| @@ -11,7 +11,7 @@ bench: | |||
| go test -bench=. -benchmem | |||
|
|
|||
| cyclo: | |||
| gocyclo -over 13 ./*/*.go | |||
| gocyclo -over 18 ./*/*.go | |||
There was a problem hiding this comment.
Please don't do this. Increasing it to make this step pass is not the ideal solution, can you find a way to refactor the functionality (break logic into new functions if possible)?
Or if it is too much to do, I don't mind ignoring this comment - I can fix it later and refactor the functions to stay within complexity limits I've set for myself.
list/render.go
Outdated
| // - Is | ||
| // - Known | ||
| // - The Dark Tower | ||
| // - The Gunslinger |
There was a problem hiding this comment.
Please undo this and all the other changes to comment blocks.
| @@ -74,7 +88,7 @@ func LongestLineLen(str string) int { | |||
| maxLength = currLength | |||
| } | |||
| currLength = 0 | |||
| } else if !isEscSeq { | |||
| } else if !isEscSeq && !isEscSeqAlt { | |||
| currLength += RuneWidth(c) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Can you add unit-tests for the changes? Call the affected functions with text containing an escape sequence with a URL as defined in the spec linked?
text/string.go
Outdated
Comment on lines
12
to
18
| EscapeReset = EscapeStart + "0" + EscapeStop | ||
| EscapeStart = "\x1b[" | ||
| EscapeStartAlt = "\x1b]8" | ||
| EscapeStartRune = rune(27) // \x1b | ||
| EscapeStop = "m" | ||
| EscapeStopRune = 'm' | ||
| EscapeStopRuneAlt = '\\' |
There was a problem hiding this comment.
I wonder if there is a better way to do this for now and the future, in case another pattern needs to be supported. But this is a good start.
|
Hi Naveen,
I'll see what I can do. I thought about refactoring, but at the time it seemed to be taking too much time.
Thanks,
Vladimir.
On Tuesday, February 21, 2023 at 05:16:53 PM EST, Naveen Mahalingam ***@***.***> wrote:
@jedib0t requested changes on this pull request.
________________________________
In Makefile:
@@ -11,7 +11,7 @@ bench:
go test -bench=. -benchmem
cyclo:
- gocyclo -over 13 ./*/*.go
+ gocyclo -over 18 ./*/*.go
Please don't do this. Increasing it to make this step pass is not the ideal solution, can you find a way to refactor the functionality (break logic into new functions if possible)?
Or if it is too much to do, I don't mind ignoring this comment - I can fix it later and refactor the functions to stay within complexity limits I've set for myself.
________________________________
In list/render.go:
-// * Is
-// * Coming
-// * This
-// * Is
-// * Known
-// * The Dark Tower
-// * The Gunslinger
+// - Game Of Thrones
+// - Winter
+// - Is
+// - Coming
+// - This
+// - Is
+// - Known
+// - The Dark Tower
+// - The Gunslinger
Please undo this and all the other changes to comment blocks.
________________________________
In text/string.go:
@@ -74,7 +88,7 @@ func LongestLineLen(str string) int {
maxLength = currLength
}
currLength = 0
- } else if !isEscSeq {
+ } else if !isEscSeq && !isEscSeqAlt {
currLength += RuneWidth(c)
}
}
Can you add unit-tests for the changes? Call the affected functions with text containing an escape sequence with a URL as defined in the spec linked?
________________________________
In text/string.go:
+ EscapeReset = EscapeStart + "0" + EscapeStop
+ EscapeStart = "\x1b["
+ EscapeStartAlt = "\x1b]8"
+ EscapeStartRune = rune(27) // \x1b
+ EscapeStop = "m"
+ EscapeStopRune = 'm'
+ EscapeStopRuneAlt = '\\'
I wonder if there is a better way to do this for now and the future, in case another pattern needs to be supported. But this is a good start.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Hello Naveen,
I have refactored the string.go code to bring the complexity down. It now passes the gocyclo verification.
Please, take a look.
Thanks,
Vladimir.
On Wednesday, February 22, 2023 at 12:56:00 PM EST, Vladimir Semichev ***@***.***> wrote:
Hi Naveen,
I'll see what I can do. I thought about refactoring, but at the time it seemed to be taking too much time.
Thanks,
Vladimir.
On Tuesday, February 21, 2023 at 05:16:53 PM EST, Naveen Mahalingam ***@***.***> wrote:
@jedib0t requested changes on this pull request.
________________________________
In Makefile:
@@ -11,7 +11,7 @@ bench:
go test -bench=. -benchmem
cyclo:
- gocyclo -over 13 ./*/*.go
+ gocyclo -over 18 ./*/*.go
Please don't do this. Increasing it to make this step pass is not the ideal solution, can you find a way to refactor the functionality (break logic into new functions if possible)?
Or if it is too much to do, I don't mind ignoring this comment - I can fix it later and refactor the functions to stay within complexity limits I've set for myself.
________________________________
In list/render.go:
-// * Is
-// * Coming
-// * This
-// * Is
-// * Known
-// * The Dark Tower
-// * The Gunslinger
+// - Game Of Thrones
+// - Winter
+// - Is
+// - Coming
+// - This
+// - Is
+// - Known
+// - The Dark Tower
+// - The Gunslinger
Please undo this and all the other changes to comment blocks.
________________________________
In text/string.go:
@@ -74,7 +88,7 @@ func LongestLineLen(str string) int {
maxLength = currLength
}
currLength = 0
- } else if !isEscSeq {
+ } else if !isEscSeq && !isEscSeqAlt {
currLength += RuneWidth(c)
}
}
Can you add unit-tests for the changes? Call the affected functions with text containing an escape sequence with a URL as defined in the spec linked?
________________________________
In text/string.go:
+ EscapeReset = EscapeStart + "0" + EscapeStop
+ EscapeStart = "\x1b["
+ EscapeStartAlt = "\x1b]8"
+ EscapeStartRune = rune(27) // \x1b
+ EscapeStop = "m"
+ EscapeStopRune = 'm'
+ EscapeStopRuneAlt = '\\'
I wonder if there is a better way to do this for now and the future, in case another pattern needs to be supported. But this is a good start.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
jedib0t
requested changes
Feb 24, 2023
text/string.go
Outdated
| @@ -21,12 +25,48 @@ var ( | |||
| rwCondition = runewidth.NewCondition() | |||
| ) | |||
|
|
|||
| type EscKind int | |||
There was a problem hiding this comment.
Can you make EscKind and EscSeq private to this package? They don't need to be public.
|
I'll fix the EscKind and EscSeq exposure and push the new commit today.
As for the comments, that change was introduced when I ran "make test" from the top level. I suspect it could be due to
changes in the "go fmt" or "go vet" formatting rules changes. Not sure.
If it's critical I can try and revert my commits and then apply the latest changes on top of that.
What do you think?
Thanks
On Friday, February 24, 2023 at 11:49:12 AM EST, Naveen Mahalingam ***@***.***> wrote:
@jedib0t requested changes on this pull request.
Thanks for the refactor. Looks awesome now.
You still haven't reverted the white-space changes to all the comment blocks. And can you address the new comment on EscKind and EscSeq?
Can you fix that please and I can merge.
________________________________
In text/string.go:
@@ -21,12 +25,48 @@ var (
rwCondition = runewidth.NewCondition()
)
+type EscKind int
Can you make EscKind and EscSeq private to this package? They don't need to be public.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Ok. I've fixed the comment formatting and reverted back to what it was prior to "go fmt" messing it up. I've also changed EscSeq and EscKind to be private to the text package. I'll add a unit test for the URL escape sequence, just to make sure it is handled correctly and then we'll be ready to merge (hopefully) :) |
…mment changes introduced by go fmt
|
Kudos, SonarCloud Quality Gate passed!
|
|
Hey Naveen, I've added some unit test cases for hyperlink escape sequences, so we should be ready to merge. |
jedib0t
approved these changes
Feb 25, 2023
|
@vsemichev I've just cut a new tag with this change: https://github.com/jedib0t/go-pretty/releases/tag/v6.4.5 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…t.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda)
Proposed Changes
The changes I introduced to the text/string.go will allow the text functions to correctly handle the hyperlink escape sequences described in https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda