pkg/cmd: inconsistent arguments ordering

[AkihiroSuda]

nerdctl/pkg/cmd/apparmor/list_linux.go

Line 32 in 35bb03e

func List(options types.ApparmorListCommandOptions, stdout io.Writer) error {

nerdctl/pkg/cmd/builder/build.go

Line 45 in 35bb03e

func Build(ctx context.Context, options *types.BuildCommandOptions, stdin io.Reader, stdout, stderr io.Writer) error {

nerdctl/pkg/cmd/image/pull.go

Line 40 in 35bb03e

func Pull(ctx context.Context, rawRef string, stdout io.Writer, stderr io.Writer, options types.PullCommandOptions) error {

Probably options should be consistently placed in the last.

stdout io.Writer should be probably right after ctx context.Context (if any). put into the options struct, and the nil value should imply os.Stdout

[djdongjin]

Personally I think we should order based on when an argument is used (first -> last, input -> output):

1. ctx should be always the 1st.
2. client should be right after ctx if there is: it's like the data source from which we get/put containers, images, etc.
3. then should be the main target, container(s), image(s): these are the things we want from (or send to) containerd.
4. then options (or stdout): options decides how we manipulate the main target.
5. then stdout (or options): after operation is done, we output the results (also often we don't need a stdout but just return bool/error).

So the above three will be like:

func List(options types.ApparmorListCommandOptions, stdout io.Writer) error

func Build(ctx context.Context, options *types.BuildCommandOptions, stdin io.Reader, stdout, stderr io.Writer) error

func Pull(ctx context.Context, rawRef string, options types.PullCommandOptions, stdout io.Writer, stderr io.Writer) error

I don't have a strong preference between 4. options and 5. stdout (or stdin). But stdout usually is the last step & be used less frequent than options & io is not very relevant to the operation (just for input/output).

But I think ctx, client, container|image|network|id|ref should take precedence over options, io.

[AkihiroSuda]

"options" should be optional, so I'd prefer options to be the last.

So my preference is func Foo(ctx, client, stdin, stdout, stderr, flag0, flag1, flag2, options) error .

We can also reconsider putting everything except ctx into options. (Sorry for going back and forth)

[djdongjin]

"options" should be optional, so I'd prefer options to be the last.

I'm okay with this :)

So my preference is func Foo(ctx, client, stdin, stdout, stderr, flag0, flag1, flag2, options) error .

I'm okay with this as well, but it doesn't have the main object (container/image/network/etc). How about func Foo(ctx, client, container(s)|image(s)|etc, stdin, stdout, stderr, flag0, flag1, flag2, options) error?

[AkihiroSuda]

It's hard to decide the position of stdio flags 😓 .

I think we should just put {Stdin io.Reader, Stdout, Stderr io.Writer} into the options struct.
These are optional, and nil values should imply os.Stdin, os.Stdout, os.Stderr .

[Zheaoli]

I think we can do this thing,

1. add a new data structure in pkg/api/types/common.go, named IO, contains {Stdin io.Reader, Stdout, Stderr io.Writer}
2. the ordering should be func New(ctx context.Context, parm1, parm2, string, options types.${command}Options, io IO)
3. we can merge refactor: pass an io.Writer to image.List #1887 first, and I will make a new PR to adjust the argument ordering one shot.

[AkihiroSuda]

I think IO should be in the Options struct

[Zheaoli]

I think IO should be in the Options struct

Actually, we have a previous discussion before #1792 (comment)

For semantic reasons, the name ${Command}Options should only be related to the command flags which has been registered. If we put the IO in the Option struct, maybe the Option needs to be renamed as ${Command}Context to keep the consensus semantic, but it will be mixed with the origin cobra context.

[djdongjin]

I'm okay with putting io.reader/io.writer directly to ${Command}Options (and keeping the ${Command}Options name) if we decide to wrap them in struct. :).

A new struct wrap IO adds a little more boilerplate code and Context is not as straightforward as Options (e.g., most codebase I saw uses Options instead of Context.

(Sometimes I feel even COMMANDCommandOptions (e.g. ContainerRemoveCommandOptions) is a little longer... what if we save Command and just call COMMANDOptions (ContainerRemoveOptions)?)

[AkihiroSuda]

For semantic reasons, the name ${Command}Options should only be related to the command flags which has been registered.

I think ${Command}Options can be be used for any optional stuff regardless to whether it is a CLI flag or not.

But I don't have a strong opinion 🙂 , as long as we can keep it consistent across all commands.

[Zheaoli]

OK, let's make the final decision

1. Rename all option from ${Command}CommandOptions to ${Command}Options, like NetworkCreateOptions etc.
2. Add a new command struct, named CommonOptions, contain stdout,stdin,stderr
3. The method argument ordering should be func Command(ctx context.Context, param1, param2 type, options ${Command}Options

How about this? Free free to ask if you have any problems.

[djdongjin]

OK, let's make the final decision

1. Rename all option from ${Command}CommandOptions to ${Command}Options, like NetworkCreateOptions etc.
2. Add a new command struct, named CommonOptions, contain stdout,stdin,stderr
3. The method argument ordering should be func Command(ctx context.Context, param1, param2 type, options ${Command}Options

How about this? Free free to ask if you have any problems.

Regarding 2, can we just put necessary stdin, stdout, stderr directly in the ${Command}Options if they're used?

1. Adding a CommonOptions means one more wrapper (e.g., opt.CommonOptions.stdout v.s. opt.stdout).
2. They're not as common as globaloption: some commands don't use any, some commands use stdout only, some use stdout/stderr and some uses all three. If a command only uses stdout and we pass a CommonOptions, it may confuse users that stderr/stdin are used somehow.

If a command only uses, e.g., stdout (similar to stderr/stdin, or any subset of the 3), we can just put a single stdout io.Writer (instead of all 3, or a wrapper) to its ${Command}Options. I feel it's less confusing and easier to use. WDYT @Zheaoli