refactor: extract main logic of image.List#1923
Conversation
| imageList, err := List(ctx, client, options.Filters, options.NameAndRefFilter) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return printImages(ctx, client, imageList, options) |
There was a problem hiding this comment.
Given that line 49-53 (client creation) will be moved to cmd side, I wonder if we should just export printImages (i.e. -> PrintImages) and let cmd.imagesAction first call List below and then call PrintImages to print.
I'm also okay to have the ListCommandHandler (naming wise, maybe just ListHandler, or ListAndPrint?) as a wrapper of the two logic (list and print).
WDYT @Zheaoli @AkihiroSuda
There was a problem hiding this comment.
and then call PrintImages to print.
If we take this route, maybe we also want to make it consistent across all the commands (i.e., if there's anything to print, cmd/nerdctl/X_Y.go should call PrintXXX exported by pkd/cmd/X/Y.go to do so).
There was a problem hiding this comment.
let cmd.imagesAction first call List below and then call PrintImages to print.
Also, if there's no single entrypoint for cmd.imagesAction to call, should we pass the whole types.ImageListOptions to both List and PrintImages? Some fields in types.ImageListOptions are only relevant to List, and some of them are only relevant to PrintImages.
davidhsingyuchen
force-pushed
the
reuse-image-list
branch
2 times, most recently
from
776e9ff to
e7342d0
Compare
|
Rebased on |
| func List(ctx context.Context, client *containerd.Client, options types.ImageListOptions) error { | ||
| var imageStore = client.ImageService() | ||
| imageList, err := imageStore.List(ctx, options.NameAndRefFilter...) | ||
| func ListCommandHandler(ctx context.Context, client *containerd.Client, options types.ImageListOptions) error { |
There was a problem hiding this comment.
Can you add a comment here? Something like
// ListCommandHandler `List` and print images matching filters in `options`.
Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>
davidhsingyuchen
force-pushed
the
reuse-image-list
branch
from
e7342d0 to
2ab87da
Compare
4 participants
Fix #1855.
The naming pattern of
ListCommandHandler(i.e., called bycmd/nerdctl) andList(i.e., contains "main logic") come from #1855 (comment). If this PR makes sense to you, to make the naming consistent, I can create a one-shot PR to rename all functions to beXXXCommandHandleraccording to #1855 (comment).Notes:
Listcomes from command reference. Not 100% sure about the comment ofnameAndRefFilter, please let me know if it's inaccurate, thanks!Signed-off-by: Hsing-Yu (David) Chen davidhsingyuchen@gmail.com