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

More anchors + new features (WIP): :hide-header:, :nested: complete, :post-process: #100

Closed

Conversation

TomerGodinger
Copy link

This adds the following features:

  1. :hide-header: option: omits the title and description from the output.
  2. :nested: complete option: produces a fuller listing with title, description, usage, short summary and then full nested details. Feel free to change the name to whatever you want if you choose to keep it. =)
  3. :post-process: path.to.module:func_name option: invokes the specified function in the specified module on the list of generated nodes for each command. The function should get the command used and the list of nodes that were generated.

This also adds quite a few new anchors for cross-referencing - namely one for each .. program:: directive, and for options and environment variables, an anchor is added per option name. For example, for a command group grp, command cmd, an option that has the flags -f and --flag and an environment variable env that provides a default for it, the following anchors will be created:

  • grp-cmd-f
  • grp-cmd-flag
  • grp-cmd-f-env
  • grp-cmd-flag-env

This has a caveat which I haven't dealt with, which is collisions.
Having an option with the flag names -f and --f would cause the same anchor to be created twice. Similarly a command called cmd with the flag --some-flag and a command called cmd-some with the flag --flag would both create the anchor cmd-some-flag. It may be a good idea to change the format so it's injective.
This actually applies to existing anchors as well, e.g. group grp + command some-cmd + option opt would yield the same option anchor as group grp-some + command cmd + option opt, if I'm not mistaken.
A decision not to support this would be valid, of course, but it's something that begs consideration at the least.

As mentioned in the title, this is not a finished, polished product.
While it does seem to work in general, I haven't tested it thoroughly, and since it adds new anchors, it currently breaks nearly all the existing tests, as they don't expect to get them.

Copy link
Member

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

A good start. Got a few suggestions inline. Tests would really help a lot here. Also, I see a couple of cases where you add things in the first patch and remove them again in the second or third one. Can you rebase and clean these up before force pushing updates?

sphinx_click/ext.py Outdated Show resolved Hide resolved
@@ -491,8 +495,6 @@ def _generate_nodes(
:returns: A list of nested docutil nodes
"""
ctx = click.Context(command, info_name=name, parent=parent)
if nested == NESTED_COMPLETE:
print("Nested with COMPLETE")
Copy link
Member

Choose a reason for hiding this comment

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

Can you avoid adding this in the prior commit, please

Copy link
Author

Choose a reason for hiding this comment

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

Oh damn, I'm really sorry, I was sure I'd removed all that stuff...
Will be removed, of course.

sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
@TomerGodinger
Copy link
Author

Thanks for all the suggestions.
I'll make the changes when I can and submit another PR (or can I update this one instead?).
Sorry for the mess. This is the first PR I've made and I'm not well-versed in how it works.

@TomerGodinger
Copy link
Author

A good start. Got a few suggestions inline. Tests would really help a lot here. Also, I see a couple of cases where you add things in the first patch and remove them again in the second or third one. Can you rebase and clean these up before force pushing updates?

Can you please tell me which cases of adding and removing the same things you're referring to?
The only such thing I've found was that debug print, which needed to disappear altogether.
I don't see anything else of the sort.

@TomerGodinger
Copy link
Author

@stephenfin Just a friendly reminder that I'm waiting for your replies before I update this PR.

Copy link
Member

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response. Let me know if anything is unclear and thanks for the reminder.

sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
sphinx_click/ext.py Outdated Show resolved Hide resolved
@TomerGodinger
Copy link
Author

I've pushed an update based on everything we've discussed.
Also fixed the tests and added another one for the complete nesting.
Tell me what you think when you can please.

@stephenfin
Copy link
Member

I've pushed an update based on everything we've discussed. Also fixed the tests and added another one for the complete nesting. Tell me what you think when you can please.

Oh, I missed this reply. Will look later today or tomorrow 🙈

@stephenfin stephenfin force-pushed the new-features branch 2 times, most recently from 28410f5 to 2701ed4 Compare June 28, 2022 15:54
@stephenfin
Copy link
Member

@TomerGodinger I reworked a few of the commits and fixed broken type checks but the general content should be the same. Could you take a look over and see if you're happy with it? Could you update the docs just to reflect the new changes? Once that's done, I think we're good to go.

PS: You can probably drop WIP from the PR title now too 😄

@TomerGodinger
Copy link
Author

Hey, sorry for the delay.
I haven't had time to do this, but I'll try to get on it as soon as I can.

Copy link

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Sorry for interfering; I was looking for something similar to hide-header and I found your PR. These are all great additions!

I believe there is a small inconsistency when using hide-header, see the comment inline.

Comment on lines +583 to +640
if hide_header:
final_nodes = subcommand_nodes

if nested == NESTED_NONE or nested == NESTED_SHORT:
section = nodes.paragraph()
self.state.nested_parse(result, 0, section)
final_nodes.insert(0, section)

else:
# Title

section = nodes.section(
'',
nodes.title(text=name),
ids=[nodes.make_id(ctx.command_path)],
names=[nodes.fully_normalize_name(ctx.command_path)],
)

sphinx_nodes.nested_parse_with_titles(self.state, result, section)

for node in subcommand_nodes:
section.append(node)
final_nodes = [section]
Copy link

@lorenzo-cavazzi lorenzo-cavazzi Aug 5, 2022

Choose a reason for hiding this comment

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

I feel the behavior here is inconsistent depending on :nested: short / full.
With short, the headers are hidden as expected (left is without hide-header)

Screenshot_20220805_152116

With full, the headers are still hidden, but the main command also disappears:
in the following example, the missing command is

renku project [OPTIONS] COMMAND [ARGS]...

Screenshot_20220805_152211

I thought this could be circumvented by combining :nested: none and :hide-header: + :nested: full and :hide-header: , but the result is slightly different since part of the header is still shown:
in the following example, the additional text is

Project commands.

Screenshot_20220805_152657

@sravel
Copy link

sravel commented Feb 17, 2023

hello I want to use the option :hide-header:
can you fix and valid merge request?

@sravel
Copy link

sravel commented Feb 6, 2024

will this request be included?

@stephenfin
Copy link
Member

@sravel Someone needs to pick this up again as I assume @TomerGodinger no longer needs this.

Add anchors to all the Click components (i.e. groups and commands),
along with another general one for options so that now using the full
path of the component with separating dashes is always a valid
reference. E.g. for a group "mygroup", command "mycommand" and option
"-myoption", all of the following work:

  :ref:`My Group <mygroup>`
  :ref:`My Command <mygroup-mycommand>`
  :ref:`My Option <mygroup-mycommand-myoption>`
  :option:`My Option <mygroup-mycommand -myoption>`
Adding :hide-header: to the directive will hide the title, description
and usage and only show the commands. This also means that the commands
will not be under a single section, but rather add a section each to
where the directive is located.

The ":nested: complete" option shows the name, description, usage, short
command overview (basically the entire ":nested: short" option) followed
by the details of the commands from the ":nested: full" option.
By providing the option ":post-process: module.path:funcion", the user
can direct the directive to run the provided function on the command and
its generated node for each command processed. This allows for full
per-command customization of the output.
@TomerGodinger
Copy link
Author

Ugh, I'm really sorry, I completely forgot about this.
I rebased, fixed the conflicts and updated the tests to recognize the new anchors, but I'm afraid I don't have the capacity right now to get back into this much beyond that.
It'll take at least several weeks for me to be available to take this up again.
You can take this up and continue without me if you'd like to, or wait for me to have some time free up for it.

Again, sorry for the holdup.

@stephenfin
Copy link
Member

@TomerGodinger No issues. There's no panic with this so I'm happy to wait.

@stephenfin stephenfin closed this May 14, 2024
@stephenfin
Copy link
Member

Closing this for now. If you're able to get back to this, please feel free to reopen.

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.

None yet

4 participants