click_repl always reparses the group's options and arguments and invokes it

[flacoste]

I'm writing a repl that looked like this:

import pathlib
from dataclasses import dataclass

import click
from click_repl import repl as click_repl


@dataclass
class MyDB:
    db_dir: pathlib.Path

   def get_entry(entry_id: int) -> str:
        # Stub for code that retrieves entry from the DB and formats it.
        return f"ENTRY {entry_id}"


@click.group(invoke_without_command=True)
@click.argument('db', type=click.Path(exists=False, file_okay=False, writable=True, readable=True, path_type=pathlib.Path))
@click.pass_context
def shell(ctx: click.Context, db: pathlib.Path) -> None:
    """Opens a shell to run commands on DB."""
    ctx.obj = MyDB(db)
    if ctx.invoked_subcommand is None:
        ctx.invoke(repl)


@shell.command()
@click.pass_context
def repl(ctx):
    click_repl(ctx)


@shell.command()
@click.argument('entry_id', type=int)
@click.pass_obj
def entry(mydb: MyDB, entry_id: int) -> None:
    """Display ENTRY_ID from the DB."""
    try:
        click.echo(mydb.get_entry(entry_id))
    except NotFound as e:
        click.secho(e, fg="red")

That code didn't work as expected. I was expecting something like this:

$ mycmd db
> entry 1
ENTRY 1

Instead, I got an exception about no command 1. Digging through the code, I discovered that "entry" is parsed as the argument to the group command, and not as the subcommand.

I also discovered that shell() will also be invoked in each subcommands invocation. My hunch is that the way click_repl dispatches to the parser isn't correct:

            with group.make_context(None, args, parent=group_ctx) as ctx:
                group.invoke(ctx)
                ctx.exit()

But I'm not familiar enough with the click API to know how we should be dispatching the subcommands. From the documentation, it seems that the proper way to delegate to a subcommand is through ctx.invoke(). But that takes a callable/command which we don't know at this point.

I was able to work around the issue by rewriting my code like this:

@click.group(invoke_without_command=True)
@click.option('--db', type=click.Path(exists=False, file_okay=False, writable=True, readable=True, path_type=pathlib.Path), help="the DB where entries are cached")
@click.pass_context
def shell(ctx: click.Context, db: pathlib.Path) -> None:
    # Only initializes ctx.obj on the first invocation. We are being called on every subcommands through click_repl
    if ctx.obj is None:
        if db is None:
            db = click.prompt("Enter the DB")
        ctx.obj = MyDB(db)
    if ctx.invoked_subcommand is None:
        ctx.invoke(repl)

Thanks,

[auvipy]

can you please come with a proposed fix of the part you think is not working correctly in this library? relevant test case to verify that will be highly appreciated

[flacoste]

Like I said, I don't know click's internal API well-enough to know how this should be fixed.

I could attempt writing a failing test documenting the expected behavior. But first I'd like to know that my expectations are the right ones. I mean, do you expect the group's options and arguments to be parsed on each subcommands in the repl? And is it expected that the group command is also invoked on each subcommand in the repl?

Also, can you point an existing test to serve as an example? Looking at the existing tests, I don't see any tests confirming that subcommands actually work from the repl. There are tests that shows that things are hooked together, but not that subcommands are actually dispatched.

[GhostOps77]

@flacoste In order to make your code work, you have to call the group function at the end of the file, with no args, to make it work

@click.group(invoke_without_command=True)
@click.option('--db', type=click.Path(exists=False, file_okay=False, writable=True, readable=True, path_type=pathlib.Path), help="the DB where entries are cached")
@click.pass_context
def shell(ctx: click.Context, db: pathlib.Path) -> None:
    # Only initializes ctx.obj on the first invocation. We are being called on every subcommands through click_repl
    if ctx.obj is None:
        if db is None:
            db = click.prompt("Enter the DB")
        ctx.obj = MyDB(db)
    if ctx.invoked_subcommand is None:
        ctx.invoke(repl)

shell()  # at the end of the file

so can u give me some failing test cases, and a potential fix?

[flacoste]

@GhostOps77 well, do you think it's actually a bug? are my expectations about how this should have worked correct? And can you point to an existing test that I can use as a starting point? I can't find any existing test making sure that registered subcommands are actually working in the repl.

[GhostOps77]

@flacoste You are right, there's no existing test cases that shows that this module can work with subcommands, (idk about [this] one, you tell me)
But from what i can see is, idk what to do, but i think the main group (i.e., shell here in your case) must be invoked by mentioning it through the sys.argv i guess (thats already too much work-around, to make it work imo), and i think that would also work if there's more than one groups that has invoke_without_command=True decorator in it

This is my idea, but as your idea goes, getting the callback function of the command is the big deal
And also if you are successful in it, there's a great difficulty in testing this module as sending user input through stdin is not possible for now. I wish there's a way for that

If thats possible, it would be easy to test your above code with our other test cases

[flacoste]

@GhostOps77, ok, yes, I should at least able to write some tests for this. I'll look at this later this week.

[flacoste]

I finally got to write those tests. They reproduce the problem pretty briefly. I'll research how to fix the underlying issue, but would welcome any pointers.

[GhostOps77]

@flacoste Thanks for doing your part
But, I actually have a fix for that, like, I've solved the command invocation part
But, now I've to make the repl suggest commands properly

To get my updated code on solving those issues, you can check the code in my repo

[flacoste]

Thanks for the pointer @GhostOps77. Looking at the context around resolve_command(), I realize that actually we should simply set the group context's protected_args to the new args, and let group.invoke() handle the dispatch. That way, it also handles chain commands and pipelines.

That means though that it's normal for the group command to be invoked on each subcommand invocation - that's part of click's behaviour. It should probably be documented though.

I've updated the tests and they are now passing with this change on the related PR #97

[GhostOps77]

@flacoste Very good! Even I haven't come up with that idea
Now all that's left is to make the REPL show tab-completions properly for this implementation
But, are you having the idea of passing the group command arguments again in the REPL? cuz i've decided to remove that thing

But without implementing that, you PR works smooth

Also, how do you think this implementation can handle chain commands and piplines?

[auvipy]

GhostOps can you also review the PR?

[flacoste]

@GhostOps77 are you talking about untested functionality? Because the existing autocompletion tests continue to work on this branch.

There is a test for chaining command (there is no test for pipelining the result, but I expect it to work as all of this is implemented within invoke() itself).

Reviewing the call sites of resolve_command(), I noticed that they were usually two code paths once for chain and one for unchained command. I thought that we didn't want to have to reimplement this, so better to just rely on click() itself via the invoke() method.

[flacoste]

@GhostOps77 sorry, I just realized that for some reasons these tests are skipped here.