Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "Edit on GitHub" links #3398

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abitrolly
Copy link
Contributor

Allow to easily jump to documentation page contents in repository.

The explanation is here https://stackoverflow.com/a/62904217
Related readthedocs/sphinx_rtd_theme#1635

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

@abitrolly abitrolly force-pushed the docs-edit-links branch 2 times, most recently from ab47c7d to c92b81c Compare December 4, 2024 10:59
@happz
Copy link
Collaborator

happz commented Dec 4, 2024

Good idea, but it's not going to be that simple. It will work for pages that exist as ReST files in the repo, and that's just a subset of pages Sphinx consumes and renders. Specification, stories, plugins, plus a couple more, all these are generated either from Python sources or, in the case of large content in specification and stories, from fmf files. So, a simple link to $PAGE.rst will not work for these because the source file literally does not exist in the repository, and is conjured from thin air by make -C docs generate run by ReadTheDocs.

https://tmt--3398.org.readthedocs.build/en/3398/spec/core.html -> 404.

Edit: on top of that, one page generated by Sphinx is often constructed from multiple fmf stories, multiple fmf files. "Edit on Github" should then most likely point to a directory rather than a single file. https://tmt.readthedocs.io/en/stable/spec/core.html -> https://github.com/teemtee/tmt/tree/main/spec/core

@abitrolly
Copy link
Contributor Author

Template uses Sphinx hasdoc function to check if the page has source. Possible solution is to let Sphinx generate the docs with extension, like https://www.sphinx-doc.org/en/master/tutorial/automatic-doc-generation.html

Or save a copy of generated sources in the repo.

@abitrolly
Copy link
Contributor Author

All right. The feature can be turned on and off on page by page basis. Setting display_github to False on generated pages should do this. Now I need to find the place where to patch this in page generator. It is also possible to generate exact URL and set github_url.

@abitrolly
Copy link
Contributor Author

abitrolly commented Dec 6, 2024

@happz
Copy link
Collaborator

happz commented Dec 6, 2024

https://tmt--3398.org.readthedocs.build/en/3398/spec/core.html is generated from https://github.com/teemtee/tmt/tree/main/spec/core but I don't see what generates it https://github.com/teemtee/tmt/blob/main/docs/Makefile

https://github.com/teemtee/tmt/blob/main/docs/Makefile#L133:

$ make docs
...
mkdir -p spec
scripts/generate-lint-checks.py templates/lint-checks.rst.j2 spec/lint.rst
scripts/generate-template-filters.py templates/template-filters.rst.j2 code/template-filters.rst
scripts/generate-plugins.py discover templates/plugins.rst.j2 plugins/discover.rst
scripts/generate-plugins.py provision templates/plugins.rst.j2 plugins/provision.rst
scripts/generate-plugins.py prepare templates/plugins.rst.j2 plugins/prepare.rst
scripts/generate-plugins.py execute templates/plugins.rst.j2 plugins/execute.rst
scripts/generate-plugins.py finish templates/plugins.rst.j2 plugins/finish.rst
scripts/generate-plugins.py report templates/plugins.rst.j2 plugins/report.rst
warn: report/reportportal.log_size_limit: could not render default value, '1 MB'
warn: report/reportportal.traceback_size_limit: could not render default value, '50 kB'
scripts/generate-plugins.py test-checks templates/plugins.rst.j2 plugins/test-checks.rst
scripts/generate-hardware-matrix.py templates/hardware-matrix.rst.j2 plugins/hardware-matrix.rst
Generating reST file for HW requirement support matrix
mkdir -p stories
scripts/generate-stories.py templates/story.rst.j2
Generating rst files from /stories/docs
Generating rst files from /stories/cli
Generating rst files from /stories/install
Generating rst files from /stories/features
Generating rst files from /stories/deferred
Generating rst files from /spec/core
Generating rst files from /spec/tests
Generating rst files from /spec/plans
Generating rst files from /spec/stories
Generating rst files from /spec/context
Generating rst files from /spec/hardware

@martinhoyer
Copy link
Collaborator

It would be cool to have it as part of #3088
Also with a matrix room button.
Just thinking aloud ;)

@abitrolly
Copy link
Contributor Author

@martinhoyer it would be nice to get proper standard for "Edit on GitHub" links in Spinx, but folks on @readthedocs probably have different priorities. I would be happy to take this as a freelance job, just to focus.

@abitrolly
Copy link
Contributor Author

# Metadata -
# https://www.sphinx-doc.org/en/master/usage/restructuredtext/field-lists.html
doc.write(
f':github_url: https://github.com/teemtee/tmt/blob/main{area}'
Copy link
Collaborator

@happz happz Dec 10, 2024

Choose a reason for hiding this comment

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

  1. The URL is invalid for forks and every branch beyond main
  2. Some areas are directories, some are fmf files. It's not important to fmf, which can handle both correctly, but here we would need to make a distinction. https://github.com/teemtee/tmt/blob/main/stories/install does not exist, https://github.com/teemtee/tmt/blob/main/stories/install.fmf does.

Every story object should have web_link() method, which returns a link to the corresponding fmf file, that would be perfect, but that's for "edit on github" per section rather than per page. My initial idea would be to collect all web links for the area stories, and find their common, shared prefix and use that one. I don't like it, but right now I don't have a better one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The URL points to main, but it should be valid as long as file path in repo doesn't change. Does it makes sense to propose edits to branches?

For .fmf vs directory, the generate script knows which template to invoke, and it can set the URL correctly. What is needed, is the list of this generated entries to check, and generators that produce them. To make sure nothing falls apart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants