-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Sort plugin names in a natural order #1166
Conversation
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.
one nit, but LGTM otherwise
cli/command/plugin/list.go
Outdated
@@ -2,11 +2,15 @@ package plugin | |||
|
|||
import ( | |||
"context" | |||
"sort" | |||
|
|||
"vbom.ml/util/sortorder" |
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.
nit: can you group this import with the other imports below?
ping @cpuguy83 |
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
cli/command/plugin/list.go
Outdated
@@ -17,6 +21,12 @@ type listOptions struct { | |||
filter opts.FilterOpt | |||
} | |||
|
|||
type byName []*types.Plugin |
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.
Can we use sort.Slice
instead of defining a type 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.
Thank you for the review. I will update the PR to use sort.Slice
cli/command/plugin/list.go
Outdated
|
||
func (n byName) Len() int { return len(n) } | ||
func (n byName) Swap(i, j int) { n[i], n[j] = n[j], n[i] } | ||
func (n byName) Less(i, j int) bool { return sortorder.NaturalLess(n[i].Name, n[j].Name) } |
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.
I'm not sure we need this lib. Should be able to compare strings directly.
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.
I think this library compares integers embedded in strings by value. In the test case here, i.e. order of names: plugin-10-foo
and plugin-2-foo
, the result of direct string compare in go is opposite of that provided by this library.
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.
Ok, makes sense.
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.
Yes; it was added for natural sort
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
89f441c
to
26151d9
Compare
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.
still LGTM
as a follow up, we can probably update the other parts of the code to use sort.Slice()
as well
@cpuguy83 good to go?
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
- What I did
Sorted the output of
plugin list
command by plugin name. fixes #1164- How I did it
Added sort functionality to
cli/command/plugin/list.go
and unit tests tocli/command/plugin/list_test.go
- How to verify it
docker plugin ls
- Description for the changelog
docker plugin ls output is sorted based on plugin name
- A picture of a cute animal (not mandatory but encouraged)