Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ezio-melotti merged 6 commits intopython:mainfromezio-melotti:cpy-file-role
Nov 14, 2022

Conversation

ezio-melotti
Copy link
Member

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

@@ -8,6 +8,12 @@


def setup(app):
# role to link to cpython files
app.add_role(
"cpy-file",
Copy link
MemberAuthor

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.

CAM-Gerlach and hugovk reacted with thumbs up emoji
@@ -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
Copy link
MemberAuthor

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

hugovk reacted with thumbs up emoji
Copy link
Member

@hugovkhugovk left a 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 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.)

CAM-Gerlach reacted with thumbs up emoji
Copy link
Member

@CAM-GerlachCAM-Gerlach left a 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
Copy link
MemberAuthor

ezio-melotti commentedOct 9, 2022
edited
Loading

One idea, filenames are often written with a fixed-width font (e.g. in thisstyle 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 (sinceMakefile 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 athttps://cpython-devguide--961.org.readthedocs.build/documentation/markup.html#roles ?

Those roles are just for thecpython repo so I don't think this should be documented. If we end up adding it topython/cpython (andpython/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 thatcpython already definesa:source: role (which is also not documented).

Click to see some related findings

Sphinx defines a bunch of extra nodes, including aliteral_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 combinesliteral andreference could be created and used as well:
https://github.com/docutils/docutils/blob/c5a6ec93fe81fc44851f65aff37bef8a4cd2d173/docutils/docutils/nodes.py#L1893-L1894

Sphinx also defines aEmphasizedLiteral 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 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
Copy link
Member

CAM-Gerlach commentedOct 9, 2022
edited
Loading

Ah, I didn't realizeautolink 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 quitepossible, 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 thefile 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.

Copy link
Member

@CAM-GerlachCAM-Gerlach left a 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 👍

@hugovk
Copy link
Member

Ah, I didn't realizeautolink 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!

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Docutils things:

A

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Copy link
Member

@CAM-GerlachCAM-Gerlach left a comment
edited
Loading

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?

@ezio-melotti
Copy link
MemberAuthor

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 tocpython and thepeps repos.

hugovk, CAM-Gerlach, and AA-Turner reacted with thumbs up emojiezio-melotti reacted with rocket emoji

Copy link
Member

@CAM-GerlachCAM-Gerlach left a 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

@ezio-melottiezio-melotti merged commitf0c9151 intopython:mainNov 14, 2022
@ezio-melottiezio-melotti deleted the cpy-file-role branchNovember 14, 2022 01:20
@ezio-melotti
Copy link
MemberAuthor

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

CAM-Gerlach reacted with thumbs up emoji

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

@AA-TurnerAA-TurnerAA-Turner left review comments

@hugovkhugovkhugovk approved these changes

@CAM-GerlachCAM-GerlachCAM-Gerlach approved these changes

Assignees

@ezio-melottiezio-melotti

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add links to files ininternals/parser.rst
4 participants
@ezio-melotti@CAM-Gerlach@hugovk@AA-Turner

[8]ページ先頭

©2009-2025 Movatter.jp