Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork860
Add a Sphinx role to link to GitHub files#961
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@@ -8,6 +8,12 @@ | |||
def setup(app): | |||
# role to link to cpython files | |||
app.add_role( | |||
"cpy-file", |
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.
Usinggh-file
will make it more symmetric withgh-label
, but since the two are not used together, I thought usingcpy-file
would make the fact that they are linking to thecpython
repo more explicit.
@@ -518,17 +518,17 @@ Pegen | |||
===== | |||
Pegen is the parser generator used in CPython to produce the final PEG parser used by the interpreter. It is the | |||
program that can be used to read the python grammar located in :file:`Grammar/Python.gram` and produce the final C | |||
program that can be used to read the python grammar located in :cpy-file:`Grammar/python.gram` and produce the final C |
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.
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 withlinkcheck
:
(internals/parser: line 520) broken https://github.com/python/cpython/blob/main/Grammar/Python.gram - 404 Client Error: Not Found for url: https://github.com/python/cpython/blob/main/Grammar/Python.gram
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.
Looks good!
One idea, filenames are often written with a fixed-width font (e.g. in thisstyle guide), shall we have the role do that too?
And document this role athttps://cpython-devguide--961.org.readthedocs.build/documentation/markup.html#roles ?
(We should addgh-label
there too.)
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.
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.
ezio-melotti commentedOct 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
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 therole level (rather than thenode level) we could subclassed that and enhance it to support linking. This shouldn't betoo difficult, but it's not entirely obvious to me how to do it and I'm happy with the current solution. |
CAM-Gerlach commentedOct 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
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 quitepossible, 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. |
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 few minor comments; otherwise LGTM for now 👍
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
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! |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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.
Docutils things:
A
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
CAM-Gerlach left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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 theReferenceRole
superclass to handle that), so I'm not sure if its worth it now.@AA-Turner any easy way to add this?
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 |
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.
SGTM, thanks@ezio-melotti
I created a new PR to use the new role throughout the Devguide: |
This PR adds a role to link to GitHub files (in the
python/cpython
repo) and uses them ininternals/parser.rst
.internals/parser.rst
#960