GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@ezio-melotti ezio-melotti commented Oct 7, 2022

Copy link
Copy Markdown
Member

This PR adds a role to link to GitHub files (in the python/cpython repo) and uses them in internals/parser.rst.

  • Fixes Add links to files in internals/parser.rst #960

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ezio-melotti added 2 commits October 7, 2022 05:56
@ezio-melotti
Add a new role to link to cpython files.
c45627f
@ezio-melotti
Use the new :cpy-file: role in internals/parser.rst.
43b0c71
@ezio-melotti ezio-melotti added the type-feature Additions; New content or section needed label Oct 7, 2022
@ezio-melotti
ezio-melotti requested a review from hugovk October 7, 2022 03:59
@ezio-melotti ezio-melotti self-assigned this Oct 7, 2022
ezio-melotti
ezio-melotti commented Oct 7, 2022
View reviewed changes
Comment thread _extensions/custom_roles.py
def setup(app):
# role to link to cpython files
app.add_role(
"cpy-file",

@ezio-melotti ezio-melotti Oct 7, 2022

Copy link
Copy Markdown
Member Author

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.

Using gh-file will make it more symmetric with gh-label, but since the two are not used together, I thought using cpy-file would make the fact that they are linking to the cpython repo more explicit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach and hugovk reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread internals/parser.rst

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

@ezio-melotti ezio-melotti Oct 7, 2022

Copy link
Copy Markdown
Member Author

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 with linkcheck:

(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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ezio-melotti ezio-melotti mentioned this pull request Oct 7, 2022
Add links to files in internals/parser.rst #960
Closed
hugovk
hugovk approved these changes Oct 7, 2022
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

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!

image

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.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
CAM-Gerlach
CAM-Gerlach reviewed Oct 7, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ezio-melotti
Make the output a literal.
e194b3e
@ezio-melotti

ezio-melotti commented Oct 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?

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 ! so that we can do e.g. :cpy-file:`!Makefile` without creating a link (since Makefile is generated, and not in the actual repo).

I also thought about adding support for variables parts like :cpy-file:`somedir/python3.{x}/somefile.py`, but this seems more involved and IMHO not worth it. If there is a variable part, the file can't be linked anyway and in that case a regular :file: can be used (even if it's slightly less semantic in the source, but otherwise unnoticeable since they render the same).

Support for ~ (to strip the dirs and leave the filename) seems YAGNI to me, but it's not too difficult to add if we really wanted to.

This is how the output of the current PR looks like:
image

And document this role at https://cpython-devguide--961.org.readthedocs.build/documentation/markup.html#roles ?

Those roles are just for the cpython repo so I don't think this should be documented. If we end up adding it to python/cpython (and python/peps too), then it can be documented. (Doing that would also mean duplicating (or triplicating) the code, so we might want to come up with a better solution.)

Note that cpython already defines a :source: role (which is also not documented).

Click to see some related findings

Sphinx defines a bunch of extra nodes, including a literal_emphasis, but it doesn't support variables parts and just makes everything italic. I haven't seen a node that already does what we need:
https://github.com/sphinx-doc/sphinx/blob/6ed4bbba396eb410c4d00e5f9881aa621cec749a/sphinx/addnodes.py#L512

A new node type that combines literal and reference could be created and used as well:
https://github.com/docutils/docutils/blob/c5a6ec93fe81fc44851f65aff37bef8a4cd2d173/docutils/docutils/nodes.py#L1893-L1894

Sphinx also defines a EmphasizedLiteral role (which is what is used for the :file: role):
https://github.com/sphinx-doc/sphinx/blob/6ed4bbba396eb410c4d00e5f9881aa621cec749a/sphinx/roles.py#L272

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti
ezio-melotti requested a review from AA-Turner October 9, 2022 05:40
@CAM-Gerlach

CAM-Gerlach commented Oct 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Ah, I didn't realize autolink was a custom role defined for this repo (I'd seen it mentioned being used on the NumPy repo and elsewhere); my assumptions and a few comments I'd seen when googling had led me to believe otherwise, and we'd have to re-implement it ourselves (in which case I went with the class approach), but don't know why I didn't double check.

In any case, yeah, its simple to add that given it's already defined here, especially for a first implementation. As for

If we end up adding it to python/cpython (and python/peps too), then it can be documented. (Doing that would also mean duplicating (or triplicating) the code, so we might want to come up with a better solution.)

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, ~ support, custom link titles (which are automatically not rendered as a literal), emulating the CSS classes of the file role, and some other things, while being fully extensible to files on any GitHub project, or indeed any arbitrary link.

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 @ instead of : for branches/tags/commits, adding URL escaping). However, it could form a common base class (and subclasses) implemented in a Sphinx extension to provide many of the similar, currently-custom roles used/desired across our repos (GitHub issue/PR, BPO issue, PEPs, and RFCs, alongside source files, which could all specify other repos or even orgs if needed), with a superset of the existing functionality, to replace the patchwork of repo-bespoke code. Going forward, building off this would likely make more sense than copying things around piecemeal between repos, and would be easily reusable for other projects as well.

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.

That's quite possible, but would require basically re-implementing the role, which is really just the same as :samp: with an different CSS class, as it is basically one big monolithic function and almost all the logic is devoted to parsing the sample bits, which aren't useful for a concrete specific file (that this role represents) as opposed to an abstract file that the file role corresponds to. Your approach here is far more reasonable, especially as a first implementation.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach
CAM-Gerlach reviewed Oct 9, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

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 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Oct 9, 2022

Copy link
Copy Markdown
Member

Ah, I didn't realize autolink was a custom role defined for this repo (I'd seen it mentioned being used on the NumPy repo and elsewhere);

Yeah, it's one of those handy bits of sample code that gets copied and pasted around a lot!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti
ezio-melotti commented Oct 9, 2022
View reviewed changes
Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti @CAM-Gerlach
Apply suggestions from code review
2e9fba3
Co-authored-by: C.A.M. Gerlach <[email protected]>
AA-Turner
AA-Turner reviewed Oct 10, 2022
View reviewed changes

@AA-Turner AA-Turner left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti @AA-Turner
Apply suggestions from code review
060b22a
Co-authored-by: Adam Turner <[email protected]>
ezio-melotti
ezio-melotti commented Oct 10, 2022
View reviewed changes
Comment thread _extensions/custom_roles.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti
Remove tabs.
3acc352
CAM-Gerlach
CAM-Gerlach reviewed Oct 11, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ezio-melotti

ezio-melotti commented Nov 13, 2022

Copy link
Copy Markdown
Member Author

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 cpython and the peps repos.

hugovk, CAM-Gerlach, and AA-Turner reacted with thumbs up emoji ezio-melotti reacted with rocket emoji
All reactions
  • 👍 3 reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach
CAM-Gerlach approved these changes Nov 13, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ezio-melotti
ezio-melotti merged commit f0c9151 into python:main Nov 14, 2022
@ezio-melotti
ezio-melotti deleted the cpy-file-role branch November 14, 2022 01:20
@ezio-melotti ezio-melotti mentioned this pull request Nov 14, 2022
Use :cpy-file: throughout the Devguide #984
Merged
@ezio-melotti

ezio-melotti commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

I created a new PR to use the new role throughout the Devguide:

  • Use :cpy-file: throughout the Devguide #984
CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner left review comments
@hugovk hugovk hugovk approved these changes
@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@ezio-melotti ezio-melotti

Labels

type-feature Additions; New content or section needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add links to files in internals/parser.rst

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ezio-melotti @CAM-Gerlach @hugovk @AA-Turner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.