-
Notifications
You must be signed in to change notification settings - Fork 57
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 (long overdue): :hide-header:, :nested: complete, :post-process: #100
Conversation
There was a problem hiding this 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
@@ -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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Thanks for all the suggestions. |
Can you please tell me which cases of adding and removing the same things you're referring to? |
@stephenfin Just a friendly reminder that I'm waiting for your replies before I update this PR. |
There was a problem hiding this 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.
I've pushed an update based on everything we've discussed. |
Oh, I missed this reply. Will look later today or tomorrow 🙈 |
28410f5
to
2701ed4
Compare
@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 |
Hey, sorry for the delay. |
There was a problem hiding this 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.
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] |
There was a problem hiding this comment.
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
)
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]...
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.
hello I want to use the option :hide-header: |
will this request be included? |
@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.
d11a908
to
88816ba
Compare
Ugh, I'm really sorry, I completely forgot about this. Again, sorry for the holdup. |
@TomerGodinger No issues. There's no panic with this so I'm happy to wait. |
Closing this for now. If you're able to get back to this, please feel free to reopen. |
@TomerGodinger or somewone can implemented this good features? |
OK, I realized that I'm probably never going to get to dive back into this, so I did enough to at least get it to a reasonable state. @stephenfin I don't know how to reopen this PR, so I'd appreciate your guidance there. I actually don't know how to get it to sync the new changes, either. The branch I made in my fork already has them, but I don't see them here. I'm sure more can and should be added here (perhaps more tests), but I don't know if and when I'll get to that. I'll try to pay more attention to this until we're done with it, but I have a pretty high load nowadays so it's problematic. I had a bit of downtime today so I wanted to make this workable again at least. Only took me two years. 😅 |
I think you probably want to open a new PR at this point. You can link it back here. I'll do my best to review it sooner rather than later, so we can finally get this closed off! |
^ Done. |
This adds the following features:
:hide-header:
option: omits the title and description from the output.: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. =):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 groupgrp
, commandcmd
, an option that has the flags-f
and--flag
and an environment variableenv
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 calledcmd
with the flag--some-flag
and a command calledcmd-some
with the flag--flag
would both create the anchorcmd-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
+ commandsome-cmd
+ optionopt
would yield the same option anchor as groupgrp-some
+ commandcmd
+ optionopt
, 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.