GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add a Sphinx role to link to GitHub files by ezio-melotti · Pull Request #961 · python/devguide Conversation
ezio-melotti
commented
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After creating the link, I (manually) found out that it was supposed to be lowercase. Upon further testing, I found out that these are already integrated with
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
hugovk
approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
CAM-Gerlach
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments; otherwise LGTM for now 👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Yeah, it's one of those handy bits of sample code that gets copied and pasted around a lot! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: C.A.M. Gerlach <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Docutils things:
A
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Adam Turner <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
SGTM, thanks @ezio-melotti
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I created a new PR to use the new role throughout the Devguide: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Add links to files ininternals/parser.rst
There was an error while loading. Please reload this page.