Skip to content

Conversation

@max-frank
Copy link
Contributor

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 max-frank marked this pull request as draft December 10, 2020 03:07
@max-frank
Copy link
Contributor Author

@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)

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@max-frank 👍 Thanks! I think we're almost there — the diff looks good, we only need to update the failing test.

I think we can drop test_custom_multicommand_name(), and instead add a parametrization on the other test to test against both an explicitly-named multi-command, as well as one where no explicit name was passed:

@pytest.mark.parametrize("multi", [
    pytest.param(MultiCLI("multi", help="Multi help"), id="explicit-name"),
    pytest.param(MultiCLI(help="Multi help"), id="no-name"),
])
def test_custom_multicommand(multi):
    ...

WDYT?

@florimondmanca
Copy link
Contributor

florimondmanca commented Dec 10, 2020

@max-frank To address your points…

  • Good question about multi sub-commands. I don't think we were being particularly cautious with that case yet, so I'd say let's put it aside for now; we can react to any bugs people surface related to that.
  • As for command.name, yes I believe it just can't be None. Note that we are sorting by cmd.name in _recursively_make_command_docs() too so if it could, I guess somebody we have reported that as a bug by now as well. 😄

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
Copy link
Contributor Author

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.

Maximilian Frank added 2 commits December 10, 2020 13:13
Due to code changes it is now impossible for custom multi commands without names
to trigger the tested exception.
If command.name is not set (should only happen if command object is initialized manually)
the input command option will be used
@max-frank max-frank marked this pull request as ready for review December 10, 2020 12:56
@max-frank
Copy link
Contributor Author

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.

@florimondmanca florimondmanca self-requested a review January 7, 2021 15:01
Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@max-frank Taking this over for the next minor release, I updated it to take into account the latest additions. This is great work, thank you!

@florimondmanca florimondmanca merged commit a6a089d into mkdocs:master Feb 19, 2021
@florimondmanca florimondmanca mentioned this pull request Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants