Fixed kubecolor coloring oc rsh
#117
Merged
Conversation
This file contains hidden or 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
applejag
commented
May 1, 2024
Comment on lines
+71
to
78
| // oc (OpenShift CLI) specific subcommands | ||
| kubectl.Rsh: {}, | ||
| } | ||
|
|
||
| return true | ||
| func isColoringSupported(sc kubectl.Subcommand) bool { | ||
| _, found := unsupported[sc] | ||
| return !found | ||
| } |
There was a problem hiding this comment.
Changing this from a slice to a map does have performance implications.
When benchmarking, a slice was as expected faster when looking up the first element in the list, but the map approach was actually faster when looking up the last element.
$ go test -bench=. ./command -benchtime=5s
goos: linux
goarch: amd64
pkg: github.com/kubecolor/kubecolor/command
cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
BenchmarkIsColoringSupported_rsh-8 489333993 16.41 ns/op
BenchmarkIsColoringSupportedMap_rsh-8 531382500 13.78 ns/op
BenchmarkIsColoringSupported_attach-8 1000000000 4.193 ns/op
BenchmarkIsColoringSupportedMap_attach-8 680426997 9.031 ns/op
PASS
ok github.com/kubecolor/kubecolor/command 29.554sHowever the code is way smaller and in my opinion the benefit of the clean code wins over the cost of some nanoseconds (btw a nanosecond is 0.000000001 seconds)
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.
Description
Disables coloring on the
rshsubcommand, which comes fromoc rsh.Could change so unknown commands don't get colored at all, which could prevent this in other commands. But for now I just went with adding
rshas a known subcommand.Type of change
What you changed
Why you think we should change it
Fixes issue for
ocusers, especially since we declareocsupport in the README now.Related issue (if exists)
Closes #108