Add a Sphinx role to link to GitHub files #961
Conversation
hugovk
left a comment
There was a problem hiding this comment.
Looks good!
One idea, filenames are often written with a fixed-width font (e.g. in this style guide), shall we have the role do that too?
And document this role at https://cpython-devguide--961.org.readthedocs.build/documentation/markup.html#roles ?
(We should add gh-label there too.)
Sorry, something went wrong.
CAM-Gerlach
left a comment
There was a problem hiding this comment.
This is very helpful; we could add this to the PEPs too.
I do agree with @hugovk that this really should be literal-formatted like the original :file: role, though I'm not entirely sure how to achieve that without just re-implementing the link manually.
Sorry, something went wrong.
It took me a bit but eventually (and with the help of @AA-Turner and @CAM-Gerlach) I figured out how to make it a literal, in a way that is simple and concise enough. While I was at it, I also added support for I also thought about adding support for variables parts like Support for This is how the output of the current PR looks like:
Those roles are just for the Note that Click to see some related findingsSphinx defines a bunch of extra nodes, including a A new node type that combines Sphinx also defines a If we wanted to handle this at the role level (rather than the node level) we could subclassed that and enhance it to support linking. This shouldn't be too difficult, but it's not entirely obvious to me how to do it and I'm happy with the current solution. |
Sorry, something went wrong.
|
Ah, I didn't realize In any case, yeah, its simple to add that given it's already defined here, especially for a first implementation. As for
I've drafted (but not yet tested, so it likely needs some debugging) a common base class that provides a superset of the existing functionality, including everything here plus customizable branches/tags/commits, It needs some more refactoring (to move the default org/repo to config settings rather than a custom subclass, and provide a factory function to generate new ones), a couple more features (support for fragments and validation so it works with PEPs/RFCs, support for GitHub issues/PRs), and a few tweaks (using
That's quite possible, but would require basically re-implementing the role, which is really just the same as Longer-term, the above proposal features a common base class with many small, overridable methods, which can be easily subclassed used for other types of links and custom behavior, which avoids having to reimplement everything. |
Sorry, something went wrong.
CAM-Gerlach
left a comment
There was a problem hiding this comment.
A few minor comments; otherwise LGTM for now 👍
Sorry, something went wrong.
Yeah, it's one of those handy bits of sample code that gets copied and pasted around a lot! |
Sorry, something went wrong.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
AA-Turner
left a comment
There was a problem hiding this comment.
Docutils things:
A
Sorry, something went wrong.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
There was a problem hiding this comment.
Seems to work well overall now. While more features can be added in the future, or it can be replaced with a more powerful, flexible and extensible alternative, IMO the main limitation here that could be worth addressing now is that there is currently no support for explicit titles/overriding the default title; i.e. :cpy-file:Python's PEG generator <Tools/peg_generator/>` displays:
Python's PEG generator <Tools/peg_generator/>
But I'm not sure how easy that would be to add aside from manual parsing logic (my longer-term approach relies on the ReferenceRole superclass to handle that), so I'm not sure if its worth it now. @AA-Turner any easy way to add this?
Sorry, something went wrong.
|
I'm going to merge this and follow up with another PR to use it with other files. We can then improve on it with other PRs and possibly port it to |
Sorry, something went wrong.
CAM-Gerlach
left a comment
There was a problem hiding this comment.
SGTM, thanks @ezio-melotti
Sorry, something went wrong.
|
I created a new PR to use the new role throughout the Devguide: |
Sorry, something went wrong.


This PR adds a role to link to GitHub files (in the
python/cpythonrepo) and uses them ininternals/parser.rst.internals/parser.rst#960