-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Infra: Add new directives to point to PEP's canonical content #2702
Infra: Add new directives to point to PEP's canonical content #2702
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.
- With both a URI and a link title passed as two arguments, it uses the latter as the title for the former:
.. canonical-content:: https://example.org Example
Normally links in RST (and MD) have the link text first, and then the link. Would it be a bit more consistent and easier to remember to follow suit?
For example:
.. canonical-content:: Example https://example.org
pep_sphinx_extensions/pep_processor/parsing/pep_canonical_content_directive.py
Outdated
Show resolved
Hide resolved
pep_sphinx_extensions/pep_processor/parsing/pep_canonical_content_directive.py
Outdated
Show resolved
Hide resolved
Alternative suggestion: this can be...
That's consistent with how links would look in reST (minus the backticks and underscores). |
…ective Co-authored-by: Hugo van Kemenade <[email protected]>
Unfortunately, that actually doesn't work for two reasons. First, There is one way around that—just pass everything all as a single argument and do our own arg parsing, error handling, etc. We could do that, but it means we have to handle all the edge cases and possible error conditions ourselves, and refactor the directive accordingly, with the attended added complexity. I'm not sure its worth it, but I can implement that if that's what people really want.
Likewise, this would require passing everything in one raw argument and then doing our own arg parsing, as well as stripping the angle brackets. Likewise, Its possible to do, but it calls into question what should be done if one or both angle brackets is missing, since they aren't syntactically or semantically required, just window dressing. And if it works without them, why pass them at all—and in the other direction, why just the angle brackets and not, e.g., the backticks or underscore? And on a broader level, I'm not sure its worth inventing our own "sorta not not really" reST syntax for something simple like this, as it might confuse people into using the actual reST link syntax, or using other reST syntax in the link title, for instance, when the title and link can simply be passed as arguments. If we really want to be clear, we could pass them as named keyword options rather than positional arguments, but that would substantially increase the verbosity and complexity of use, since people would have to remember and type the option names. |
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.
Thanks for checking, fine by me to keep it simple then!
Arriving at this discussion a little late. I have two broad comments -- firstly that now we're using Sphinx it is better to inherit from Sphinx's directive helpers (this could be a later refactor though), and secondly that I think we should have the argument as reST -- it is fairly easy to use e.g. Having the argument as reST would allow all three forms (nothing, bare link, link with text), and I think would be less surprising. A |
Sure, your suggestions are particularly welcome on that front—I just hacked the implementation together and I figured you'd have some input on how to improve it, given your depth of Sphinx expertise. Feel free to either make review suggestions here or a followup PR, whichever you'd prefer.
Yeah, I thought about that—it shouldn't be hard or complex to implement, and might actually even simplify it a little. On the other hand, it does mean people have to use the reST link syntax instead of the link and title, and it will allow them to have unsightly, verbose and non-accessible bare links. Also, if they make a syntax mistake, they either get a possibly confusing error or the link silently doesn't work as intended, and in fact, it isn't entirely trivial to check if there's even a link at all—though this does allow more flexibility in what gets injected for special cases. That said, if its mainly just us using it rather than individual authors, pretty much all of this is moot either way, and for us full reST is likely easier to remember/use and more powerful than individual arguments, so long as we're diligent about it. @hugovk @pradyunsg @brettcannon what do you think?
Just to note, so does the current form—see examples above. Though, now that I think of it, there's no reason not to require a real link title if a URI is provided, especially since its likely going to be mostly us using it anyway. |
Yeah, this sounds good. |
I'm on board with allowing raw reST directly as well. :) |
5560ffc
to
1a9162e
Compare
I've reworked this PR to add three custom directives intended to be used at the top of PEPs:
The revised form of the directive accepts an optional arbitrary reST argument, such as a Here's how various source forms map to rendered output: |
Overall, its not ideal as the syntax is more complex, and it means users can theoretically pass bare links, but it is standard reST and allows the full power thereof. It also allowed for some significant refactoring and simplification, and intersphinx is not just useful here but potentially many other places too. |
Thanks, and for the screenshot! Would it be useful to add banners/admonitions either to PEP 12 or a demo page (linked from PEP 12)? Would help for testing/reviewing, and for authors to see what's available and what it looks like. |
Yeah, I actually had a "pep-9999.rst" demo page I keep around for this purpose internally (that's where the screenshot comes from), that also has all the admonition banners from #2691 as well. In fact, we really should add a section to PEP 12 that explains how to use the banner alongside the examples. If we end up making the banner "sticky", it won't really work to have it in the middle of the PEP or have multiple so we'd have to exclude it via e.g. a custom class, but we can deal with that if and when that happens. Accordingly I've updated PEP 12 with a new |
Looks good, thanks! |
@brettcannon @pfmoore @pradyunsg Any last concerns with the wording of the banner? |
Just a style nitpick: instead of using the semicolon, the first paragraph could be split into two sentences. |
No "last concerns with the wording of the banner" from me. |
FYI, I'm already planning to use the intersphinx links to allow directly, robustly and easily linking to the PyPA specs and glossary in PEP 639. This will also make conversion of the relevant PEPs to Python documentation, PyPA specs, etc. simpler and more straightforward to both execute and review (reducing issues like pypa/packaging.python.org#1111 ) by minimizing the deltas between them, since they will now be able to use all the same references and roles. I'll go ahead and merge in 24 hours if we don't hear from anyone else, so we can move ahead with that—we can always iterate on the text further, especially since this doesn't actually add the banner to any PEPs yet. I'll then rebase #2690 on this and replace the bespoke admonitions with these to actually implement them for the PyPA packaging PEPs. It sounded like @pfmoore might have had some potential concerns on a previous iteration in #2690 , and if that's still the case we could tweak the PyPA text further on that PR if need be, which merging this would unblock. |
Also, pushed a commit to split the preface into two sentences, as @encukou requested (as well as use more logical and easier to follow line breaks in the string parts). |
Its been 7 days since last call, this doesn't change anything on its own and we can always iterate further from here, so going ahead and merging this now so we can see how it looks on #2690 , etc. |
As originally discussed in #2692 , this adds a three new custom directives intended to be used at the top of PEPs:
pep-banner
, a generic admonition banner that can be subclassed with an arbitrary message,canonical-docs
, which renders a banner linking to aFinal
PEP's canonical documentation/specification.canonical-pypa-spec
, a banner for packaging interoperability PEPs linking to the canonical PyPA specs, which I will rebase Packaging PEPs: Update headers & link canonical PyPA specs #2690 on once this is merged.If needed in the future (e.g. for Typing), it is easy to add additional subclasses as needed that simply change the text.
Following the feedback below, the revised form of the directive accepts an optional arbitrary reST argument, such as a
:ref:
or link, which gets injected into the banner content at the appropriate point. It also adds intersphinx support for the PyPUG and the CPython documentation, for easier and more robust cross-references both within the banner and in PEPs, without having to hardcode full URIs in every PEP.Here's how various source forms map to rendered output:
Previous argument-based usage
Expand for details
With no arguments and no custom content, it produces a default message with a link to the Python docs:
.. canonical-content::
With custom content but no arguments, it produces a generic pre and post-message, allowing multiple documents to be linked and custom text to be added:
With a URI passed as a single argument, it inserts it into the message instead with a default link text:
With both a URI and a link title passed as two arguments, it uses the latter as the title for the former:
Finally, all this can be combined:
The PyPA spec subclass, aside from different text, has slightly modified behavior; it needs only a document name on the PyPA spec site rather than a full URI, and uses the same pre-text when no arguments are passed whether or not content is present. Thus, the following results:
Expand for details
Resolves #2692