Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter nested commands #133

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aeisenbarth
Copy link
Contributor

Summary

The commands option currently allows to filter commands only at the first nesting level. When CLIs have deeper level of subcommands there is no way to filter or order them apart from writing a directive for each subcommand.

This change passes the commands option to nested commands. As previously, if a command name at the CLI's top level occurs in the option, it is not documented. If a command name at any deeper level occurs in the option, it is now also excluded.

Tasks

  • Added unit tests
  • Added documentation for new features (where applicable)
  • Added release notes (using reno)
  • Ran test suite and style checks and built documentation (tox)

Further details

  • The filtering option does not distinguish the level of commands. This keeps the code and interface simple, but it could lead to a collision if a subcommand has the same name as a command but the user wants to filter only at a certain level. An alternative would be to provide full commands to filter, for example

    :option: 'command1 subcommand1', command2
    

    (or with another delimiter like command1.subcommand1)

  • I considered adding a unit test in CommandFilterTestCase. However all those tests test ext._format_command which does not include nested commands at deeper levels (nested=full). For full nested commands, it is called several times by ClickDirective._generate_nodes.

    So I extended the existing full test case with the :commands: option.

    If there is a better way to test string formatting of nested=full, or if you prefer to keep the existing full test unchanged and add a separate one with commands filtering, I am happy to change that.

@stephenfin
Copy link
Member

Those test failures look real. I do wonder if this is really the best way to do this though, and I could envision things getting out of hand relatively quickly. I'm assuming most people are not going more than 3-4 levels deep. This being the case, couldn't you simply document the top level command with :nested: none and then document the subcommands with :nested: full and :commands: {commands filter}? It's a bit of duplication in your docs but unless you've dozens of commands what harm (and if you do, a super long :commands: option probably isn't the way). I'd also be open to adding other :nested: values to allow e.g. full display but only for the first level of subcommands.

@stephenfin
Copy link
Member

I'd also be open to adding other :nested: values to allow e.g. full display but only for the first level of subcommands.

There's some prior art for this at #109, which I really need to review.

@aeisenbarth
Copy link
Contributor Author

Those test failures look real.

You are right, unfortunately the actions log does not show what exactly failed. I locally got ruff/mypy failures and was worried how they could be caused by my changes, but then I checked the master commit (before mine) and they happened there already. I have not yet looked into how to fix them.

This being the case, couldn't you simply document the top level command with :nested: none and then document the subcommands with :nested: full and :commands: {commands filter}?
In principle, that way also works, just adds some extra code and repetition (although automated is always a bit nicer).
But it would leave the inconsistency that :nested: with other options works differently at the first vs. deeper levels. Other users might encounter the same situation.

@stephenfin
Copy link
Member

Those test failures look real.

You are right, unfortunately the actions log does not show what exactly failed. I locally got ruff/mypy failures and was worried how they could be caused by my changes, but then I checked the master commit (before mine) and they happened there already. I have not yet looked into how to fix them.

The mypy failures are fixed on master now. The tests failures are another matter though 😄

@aeisenbarth
Copy link
Contributor Author

Indeed! And it was a tricky one!

The single test alone passes, but when all tests are run together, one fails. That is because sphinx-click's _load_module tries to load a module of the same name "greet" for each test from a different root. But once it has been loaded for root/basics, it is taken from the cache afterward. The other tests used to pass with the wrong greet module because the implementation was almost equal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants