[PATCH 3 of 3 PoC] cli: convert 'gearbox make-config' to 'kallithea config create'

[Thomas De Schampheleire]

El mié., 26 sept. 2018 a las 13:01, Mads Kiilerich
(<mads at kiilerich.com>) escribió:
>
> On 9/24/18 10:45 PM, Thomas De Schampheleire wrote:
> > # HG changeset patch
> > # User Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
> > # Date 1537821464 -7200
> > #      Mon Sep 24 22:37:44 2018 +0200
> > # Node ID 3c33619af70545eb9b3cbba585181094df67afbb
> > # Parent  b807b1e90e61d2c8c16c5e3ec5978ef7dd7a2d31
> > cli: convert 'gearbox make-config' to 'kallithea config create'
>
>
> Intuitively, I think I would find 'kallithea-config --create' more
> obvious. But that might just be a habit. I guess it makes sense with
> separate "sub-command area" and "sub-command" ... and then "--arguments"
> to that. So ... just a thought ...

Comparing

kallithea config create
kallithea config --create

I feel the first should be chosen. In particular if we have multiple
commands within a topic, say (hypothetically)
kallithea config create
kallithea config update
vs
kallithea config --create
kallithea config --update

Suppose that these commands create/update also need some options,
albeit different ones. Then you'd get:
kallithea config create --foo --bar
kallithea config update --baz
vs
kallithea config --create --foo --bar
kallithea config --update --baz

and that second set seems odd to me: foo/bar relate to the 'create'
part, not to the 'config' thing.

We could organize things differently, e.g. instead of one global
'kallithea' command with topics, you could create multiple global
commands, e.g.
kallithea-config
kallithea-db
kallithea-frontend
kallithea-repo
...

or in an other directory keep one global command but flatten the rest, e.g.

kallithea config-create
kallithea config-update
kallithea db-create
kallithea db-upgrade
kallithea front-end-create
etc.

>
> The main remaining doubt is as mentioned before, if the bare command
> name "kallithea" is misleading.
>
>
> > Notes regarding the original implementation:
> > - show_defaults was trying to obtain a custom
> > value from mako_variable_values, which would never happen because
> > the argument parser would not allow passing custom key-value pairs in
> > combination with '--show-defaults'.
> >
> > - variables TMPL_FILE and here were no longer used
>
>
> Right. I will land fixes for these issues. You can probably just take
> your side when rebasing.
>
>
> > TODO: documentation update
> >
> > diff --git a/kallithea/bin/kallithea_cli.py b/kallithea/bin/kallithea_cli.py
> > --- a/kallithea/bin/kallithea_cli.py
> > +++ b/kallithea/bin/kallithea_cli.py
> > @@ -14,6 +14,7 @@
> >
> >   import click
> >
> > +from kallithea.bin.kallithea_cli_config import config
> >   from kallithea.bin.kallithea_cli_frontend import frontend
> >
> >   @click.group()
> > @@ -21,4 +22,5 @@ def cli():
> >       """Various commands to set up a Kallithea instance."""
> >       pass
> >
> > +cli.add_command(config)
> >   cli.add_command(frontend)
> > diff --git a/kallithea/lib/paster_commands/make_config.py b/kallithea/bin/kallithea_cli_config.py
> > rename from kallithea/lib/paster_commands/make_config.py
> > rename to kallithea/bin/kallithea_cli_config.py
> > --- a/kallithea/lib/paster_commands/make_config.py
> > +++ b/kallithea/bin/kallithea_cli_config.py
> > @@ -11,38 +11,40 @@
> >   #
> >   # You should have received a copy of the GNU General Public License
> >   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > -"""
> > -kallithea.lib.paster_commands.make_config
> > -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > -make-config gearbox command for Kallithea
> > -
> > -:license: GPLv3, see LICENSE.md for more details.
> > -"""
> > -
> > -
> > +import click
> >   import os
> > -import sys
> >   import uuid
> > -import argparse
> >   from collections import defaultdict
> >
> >   import mako.exceptions
> >
> > -TMPL = 'template.ini.mako'
> > -here = os.path.dirname(os.path.abspath(__file__))
> > -
> > -from kallithea.lib.paster_commands.common import BasePasterCommand
> >   from kallithea.lib import inifile
> >
> > + at click.group()
> > +def config():
> > +    pass
> >
> > -class Command(BasePasterCommand):
> > -    """Kallithea: Create a new config file
> > +def show_defaults(ctx, param, value):
> > +    if not value or ctx.resilient_parsing:
>
>
> That one is not entirely obvious ... and especially why a bare return si
> the right response. Should it have a comment with an explanation ... or
> a hint about what to look for in the documentation?
>

Yes. The corresponding docs are;
http://click.pocoo.org/6/options/#callbacks-and-eager-options
"The resilient_parsing flag is applied to the context if Click wants
to parse the command line without any destructive behavior that would
change the execution flow. In this case, because we would exit the
program, we instead do nothing."

>
> > +        return
> > +
> > +    for key, value in inifile.default_variables.items():
> > +        click.echo('%s=%s' % (key, value))
> >
> > -    make-config is part of a two-phase installation process (the
> > -    second phase is setup-app). make-config creates a bare configuration
> > -    file (possibly filling in defaults from the extra
> > -    variables you give).
> > +    ctx.exit()
> > +
> > + at config.command()
> > + at click.option('--show-defaults', callback=show_defaults,
> > +              is_flag=True, expose_value=False, is_eager=True,
> > +              help='Show the default values that can be overridden')
> > + at click.argument('config_file', type=click.Path())
> > + at click.argument('key_value_pairs', nargs=-1)
> > +def create(config_file, key_value_pairs):
> > +    """Create a new configuration file
> > +
> > +    This command creates a bare configuration file (possibly filling in
> > +    defaults from the extra variables you give).
>
>
> What does "bare" mean here? It is actually quite verbose in my opinion.
>

I guess you are aware that this comes from the original code.
I think the intention of 'bare' is to say: without any real user
modification yet (except for extra variables passed).
I think the word can be removed without problem.

/Thomas