Re: [BuildStream] bst CLI design and consistency (UI)

[Chandan Singh <chandan chandansingh net>]

Hi Jürg,

On Tue, Dec 4, 2018 at 7:23 AM Jürg Billeter <j bitron ch> wrote:
> Hi Chandan,
>
> On Fri, 2018-11-30 at 14:44 -0500, Chandan Singh wrote:
> > Design principles
> > =================
> >
> > - Consistency: Following the same patterns across various subcommands, and
> >   within the same command when used in various ways.
> > - Intuitive: Hard to define, but I would say that the name of the command and
> >   the options should be enough to provide a basic understanding of what the
> >   command does.
> > - Minimal top-level: With this point I mean that we should avoid having too
> >   many top-level commands. Related commands should be grouped together in a
> >   subcommand, where applicable.
> > - Avoiding surprises: Commands should not do unexpected things.
> > - Brevity: Users should not need to type excessive amounts of text to do the
> >   necessary operations.
>
> Thanks for writing this up. These principles sound sensible to me.

Thanks for taking the time to respond.

> > Proposed changes
> > ================
> >
> > `bst artifact` subcommand
> > -------------------------
> >
> > As was already proposed before in a previous ML thread [3], the `checkout`,
> > `pull` and `push` should be folded into the `bst artifact` subcommand.
>
> Makes sense to me.
>
> > `bst source` subcommand
> > -----------------------
> >
> > Similar to how we have the `artifact` subcommand to deal with artifacts
> > (and `workspace` subcommand to deal with workspaces), maybe we should also have
> > a `source` subcommand to deal with sources. I would propose to move `fetch`,
> > `source-checkout` (as `source checkout`) and `track` under the `source`
> > subcommand.
>
> Makes sense to me as well.
>
> Does it still make sense to use `fetch` for sources and `pull` for
> artifacts or would it make sense to use `fetch` for both now that they
> no longer conflict?
>
> Transition
> ----------
>
> I'm also wondering whether we should help existing users a bit and
> provide a nicer experience than just, e.g., 'Error: No such command
> "track"'. We could keep the old top-level names as aliases with a
> deprecation warning, or at least provide an error message with the new
> command name.

A couple of questions to decide before we can proceed with this:

1. Click commands can be marked as "hidden", which will hide them from the help
   text output. But, this feature is only available since Click 7.0 that was
   released in September earlier this year. Are we okay with declaring
   Click 7.0 as a minimum requirement?

   If we were to do this, _frontend/cli.py would look something like what I've
   done in this commit:
   https://gitlab.com/BuildStream/buildstream/commit/4502a6d94193918655299fcd4be12acc0e8987d0

   If not, we won't be able to hide them from the output but we can plaster
   the word "deprecated" on their names and short help.

2. Reg. old top-level names, seems like we have a few options:

   a. Mark them as aliases of new commands, but print a deprecation warning
   b. Provide an error message referring to use the new command

   Keeping in mind that there are other CLI changes that are happening in this
   release (like workspace rework, artifact etc.), it would make sense to
   pick one approach and stick to it. So, which one do people prefer?

   I am leaning towards 2b as it attempts to teach users the new behavior
   rather than fixing it from them, and has lower burden on the mainteiners,
   but I don't really have strong opinions either way.

> Don't know whether that would be straight forward with Click. E.g., we
> probably want to remove the old commands in any case from the top-level
> help.
>
> > Unified way of dealing with multiple elements
> > ---------------------------------------------
> >
> > At present, all BuildStream commands support multiple elements with the
> > exception of `checkout`, `source-checkout` and `shell`. For all these commands
> > we already have issues to support multiple elements. Since all commands are
> > planned to support multiple elements, it would be really nice if they all were
> > consistent in the way that they do it.
> >
> > Due to limitations of `Click`, and to a certain degree to avoid ambiguity, it
> > seems like the way forward is to have only one kind of positional
> > argument, i.e. ELEMENTS. I would propose that everything else be supplied via
> > optional arguments (like --option).
>
> Yes, this makes sense to me. I'd like to add that I think the `bst
> artifact` command group should accept both element names and artifact
> refs as positional arguments. I've mentioned this already on !920¹

Definitely agree.

> Cheers,
> Jürg
>
> ¹ https://gitlab.com/BuildStream/buildstream/merge_requests/920#note_120803409

Cheers,
Chandan