mkdocs / mkdocs-click

An MkDocs extension to generate documentation for Click command line applications
https://pypi.org/project/mkdocs-click
Apache License 2.0
109 stars 15 forks source link

Use prog_name for usage #24

Closed max-frank closed 3 years ago

max-frank commented 3 years ago

This PR ensures the prog_name (introduced by #8 and initially requested in #6) is also used in the usage sections.

This is achieved by properly setting the info_name for the click.Context used during documentation rendering. Setting the info_name allows click to properly populate the ctx.command_path property and thus _docs.py line 77 now actually combines the command path and the pieces correctly. In the previous version ctx.command_path was always empty. This makes the custom command path building logic obsolete.

Now as a side effect this produces one failing test:

    def test_custom_multicommand_name():
        """Custom multi commands must be given a name."""
        multi = MultiCLI()
        with pytest.raises(MkDocsClickException):
>           list(make_command_docs("multi", multi))
E           Failed: DID NOT RAISE <class 'mkdocs_click._exceptions.MkDocsClickException'>

tests/unit/test_docs.py:108: Failed

The test obviously fails as the exception is not raised anymore in the usage portion. Now I am unsure if this test can be removed or not. For entrypoint level MultiCli() commands we get a guaranteed name populated either through the prog_name option directly or indirectly via the command option. This is regardless whether the actual MultiCli was instantiated with a name or not.

Now while I strongly believe this makes it impossible for entrypoint level multi commands to cause any name issues I am not a 100% sure about multi commands which are sub commands. If such a sub command is possible then a check+raise would have to be added to the recursive call.

    for command in sorted(subcommands, key=lambda cmd: cmd.name):
        yield from _recursively_make_command_docs(command.name, command, parent=ctx, level=level + 1)

Here command.name is invoked. Now from what I can see and know about click it is not possible to add a multi command to a click group without assigning a name to it. e.g., add_command will raise a TypeError if both the function input name and input cmd.name are None.

Please verify the correctness of my "findings" and advise if the failing test can be removed and the PR is ok as is or if it is possible to get a sub command without name. Once you have verified this feel free to remove the test or apply changes yourself or I can also do so myself if you give notify me with a comment.

max-frank commented 3 years ago

@florimondmanca I've marked this as Draft still but see description above for approval regarding testcase removal. (Adding the comment here since I am not sure how GitHub does notifications for draft PRs)

florimondmanca commented 3 years ago

@max-frank To address your points…

So overall I'd say this is all good. I'm suggesting keeping the "explicit multicommand name" in mostly as a protection against regressions, but I'd agree it's not as important either now that we use info_name=....

max-frank commented 3 years ago

I'm suggesting keeping the "explicit multicommand name"

With this do you mean the passing test_custom_multicommand test? If yes then I agree this test should be kept.


On another note I took another look at click and I am now fairly certain that sub commands will always have a name. (As you said the sort would have caused a bug by now otherwise).

For entrypoint (top) level commands it seems click itself does not seem to care if they have a name or not. Default behavior seems to be to use the invoking scripts name. Meaning for entrypoint scripts it will be the scripts name. For module invocation e.g. something like this:

if __name__ == "__main__":
    from .cli import cli
    cli()

invoked with python -m app will also use __main__ as the name which is commonly overriden by passing prog_name in the invocation. The click.testing defaults the name to root.

So with this in mind how do do you want to go about keeping "explicit multicommand name". Rather than adding logic that fails on missing top level name I'd suggest to use a meaningful default fallback instead. So something like this

@@ -19,10 +19,14 @@ def replace_command_docs(**options: Any) -> Iterator[str]:

     module = options["module"]
     command = options["command"]
-    prog_name = options.get("prog_name", command)
+    prog_name = options.get("prog_name", None)
     depth = int(options.get("depth", 0))

     command_obj = load_command(module, command)
+    name = command_obj.name
+
+    # alternatively instead of the import attr (command) 
+    # we could also default to "root" similar to click.testing
+    prog_name = prog_name or name or command

     return make_command_docs(prog_name=prog_name, command=command_obj, level=depth)

With this change I would remove the failing test_custom_multicommand_name test and add 2 new extension tests that check this new defaulting behavior.

max-frank commented 3 years ago

I have now added the above suggested change in the prog_name default behavior to the PR (including additional tests). If you rather keep the previous default behavior I can revert the PR to the previous commit.