click-contrib / sphinx-click

A Sphinx plugin to automatically document click-based applications
MIT License
212 stars 57 forks source link

_get_lazyload_commands is is passing the multicommand to itself instead of the Context #70

Closed n8pease closed 2 years ago

n8pease commented 3 years ago

The signature for click.MutliCommand.list_commands takes a Click.Context:

class MultiCommand:
...
    def list_commands(self, ctx):
        """Returns a list of subcommand names in the order they should
        appear.
        """

click.MultiCommand.get_commands is similar: get_command(self, ctx, cmd_name)

The function in sphinx-click to get commands from a multicommand is passing the multicommand to itself (instead of the Context) in list_commands and get_commands:

def _get_lazyload_commands(multicommand):
    commands = {}
    for command in multicommand.list_commands(multicommand):
        commands[command] = multicommand.get_command(multicommand, command)

    return commands

Instead, it should take the context from its caller and pass it to list_commands:

def _get_lazyload_commands(ctx):
    commands = {}
    for command in ctx.command.list_commands(ctx):
        commands[command] = ctx.command.get_command(ctx, command)

    return commands

I'm using click version 7.0, and can see the same code in the latest master (297150c7566a38fb672828a36b681792513d99b0).

My sphinx-click does not have a __version__ attribute, but I can see the same code cited above in the latest master (6b928ee1b83e3a554d040d19e95bd3063e464c60)

regisb commented 2 years ago

I am also observing this issue. In my project, when I attempt to generate docs, I get:

make build BUILD_ARGS="-W"
make[1]: Entering directory '/home/regis/projets/overhang/repos/overhang/tutor/docs'
sphinx-build -b html -a -E -n -W "." "_build/html"
Running Sphinx v4.2.0
building [mo]: all of 0 po files
building [html]: all source files
updating environment: [new config] 42 added, 0 changed, 0 removed
reading sources... [ 57%] reference/cli/tutor                                                                                                                                                                      
Exception occurred:
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/commands/cli.py", line 49, in get_commands
    actions.do_action_once("root:load", ctx.params["root"])
TypeError: list indices must be integers or slices, not str
The full traceback has been saved in /tmp/sphinx-err-ien2tje3.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make[1]: *** [Makefile:5: build] Error 2
make[1]: Leaving directory '/home/regis/projets/overhang/repos/overhang/tutor/docs'
make: *** [Makefile:8: html] Error 2

It is occurring as I am trying to implement a custom MultiCommand class to load subcommands from plugins, very much as described in the click docs: https://click.palletsprojects.com/en/latest/commands/#custom-multi-commands

You can see my custom implementation here: https://github.com/overhangio/tutor/pull/599/files#diff-135f3cab863e666db1ab4b3c1a36f125f11501065e0cae258d73d706b5dfae56R30

The context argument (which is here a MultiCommand) that is passed to list_command and get_command is not used by the base Group implementation. Thus, the bug does not occur in most cases.

The fix should be relatively straightforward and I can open a PR. Would you be interested? Is a unit test necessary?

stephenfin commented 2 years ago

I'd definitely be interested. Tests would be appreciated since they prove things are working as expected. Ideally there would be two commits: one to add (failing) unit test(s) and one to fix the bug and the test. Just try your best and let me know if you really can't figure it out.

stephenfin commented 2 years ago

Fixing this would likely unblock #83 also.

regisb commented 2 years ago

Unfortunately the proposed fix does not 100% resolve this issue. The context must also be passed to get_command, as suggested above. I opened #101 to resolve this.