-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
added ANSI color parsing and displaying colors in preview (closes #29) #154
Conversation
Thanks for your contribution! Since I don't have any time to take a code review, so I'll check this PR later 🙇♂️ |
No worries, let me know if you'd like me to write tests for this 👍 |
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 for submitting this PR! I left some comments.
fuzzyfinder.go
Outdated
@@ -263,6 +264,67 @@ func (f *finder) _draw() { | |||
} | |||
} | |||
|
|||
func parseColor(rs *[]rune) (tcell.Color, bool, error) { | |||
// only parses for 16 colors | |||
re, errCompile := regexp.Compile(`\[[0-9;]*m`) |
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.
Since Compile
is a slow function, it should be put as a global variable.
fuzzyfinder.go
Outdated
@@ -263,6 +264,67 @@ func (f *finder) _draw() { | |||
} | |||
} | |||
|
|||
func parseColor(rs *[]rune) (tcell.Color, bool, error) { | |||
// only parses for 16 colors | |||
re, errCompile := regexp.Compile(`\[[0-9;]*m`) |
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.
Maybe, we can reduce calls of regexp
functions.
s := `\[31mfoo`
re := regexp.MustCompile(`(\\\[[0-9;]+m).*`)
subStrs re.FindSubmatch([]byte(s))
if len(subStrs) == 0 {
// no matching
}
fmt.Println(subStrs[1]) // \[31m
fmt.Println(strings.TrimPrefix(s, string(subStrs[1]))) // foo
fmt.Println(strings.TrimSuffix(strings.TrimPrefix(string(subStrs[1]), `\[`), "m")) // 31
regexps for each color can also be replaced with the compare the colors.
switch n {
case "30":
return tcell.ColorBlack, ...
case "31":
return tcell.ColorRed, ...
...
}
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 for the feedback! I've pushed a new commit with the changes you suggested.
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 for the quick fix! I left some nit comments 🙏
fuzzyfinder.go
Outdated
errEntered = errors.New("entered") | ||
ErrAbort = errors.New("abort") | ||
errEntered = errors.New("entered") | ||
colorsRegex, _ = regexp.Compile(`\[[0-9;]*m`) |
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.
colorsRegex, _ = regexp.Compile(`\[[0-9;]*m`) | |
colorsRegex = regexp.MustCompile(`\[[0-9;]*m`) |
fuzzyfinder.go
Outdated
@@ -263,6 +265,53 @@ func (f *finder) _draw() { | |||
} | |||
} | |||
|
|||
func parseColor(rs *[]rune) (tcell.Color, bool, error) { |
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.
Error is never returned 👀
func parseColor(rs *[]rune) (tcell.Color, bool, error) { | |
func parseColor(rs *[]rune) (tcell.Color, bool) { |
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.
Oops, forgot to get rid of that 😅
Thanks! Should be good now.
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
==========================================
- Coverage 85.67% 82.55% -3.13%
==========================================
Files 5 5
Lines 775 837 +62
==========================================
+ Hits 664 691 +27
- Misses 97 132 +35
Partials 14 14 |
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, thanks for your contribution 👍
This is my first PR on Github so feedback is more than welcome.