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

[docs] Integrate sphinx-issues into the Sphinx config #12616

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

Conversation

shenxianpeng
Copy link
Contributor

@shenxianpeng shenxianpeng commented Apr 9, 2024

closes #12551

  1. removed extlinks of issue and pull, only left pypi because sphinx-issues not support role pypi now, but I have created Add new role :pypi: sloria/sphinx-issues#145 to let it supports
  2. changed pull to pr in NEWS.rst to fix the error NEWS.rst:3614: ERROR: Unknown interpreted text role "pull".

@shenxianpeng shenxianpeng marked this pull request as ready for review April 9, 2024 07:16
@webknjaz
Copy link
Member

webknjaz commented Apr 9, 2024

only left pypi because sphinx-issues not support role pypi now, but I have created Add new role :pypi: sloria/sphinx-issues#145 to let it supports

I have a feeling that request will be rejected as being out of the scope. The extension seems to be focused on the issue trackers.

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Apr 10, 2024

I feel the same. It would be more widely applicable if it could be changed to sphinx-links or similar, but I don't think that's possible.

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Apr 15, 2024

My pull request sloria/sphinx-issues#145 to support role pypi has been accepted and is available in the latest release of sphinx-issues (v4.1.0), so I removed the last extlinks from conf.py.

@shenxianpeng
Copy link
Contributor Author

Now it's ready for review.

@ichard26
Copy link
Member

I'm not convinced that it's worth the extra dependency (granted it's tiny) to remove only a few lines of static Sphinx configuration, but I suppose standardization of the role names (and any new ones) could be a benefit?

I'll note that issues_pr_prefix = "PR #" is needed to match the pre-existing behaviour 100%.

Ultimately I'm neutral on the change.

@webknjaz
Copy link
Member

@ichard26 yes, standardization is key. Plus, it's one thing less to copy over through many projects for people who tend to maintain many and sync them periodically. It also exposes a few other roles for free.

@webknjaz
Copy link
Member

@shenxianpeng now that you removed the entire setting, could you also drop the extension from the extensions variable so it's not even loaded?

@shenxianpeng
Copy link
Contributor Author

@webknjaz I have dropped sphinx.ext.extlinks from extensions in commit babf447

@webknjaz
Copy link
Member

@pradyunsg easy merge?

]

# General information about the project.
project = "pip"
copyright = "The pip developers"
issues_default_group_project = "pypa/pip"
Copy link
Member

@pradyunsg pradyunsg Jul 13, 2024

Choose a reason for hiding this comment

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

Please move this in its own section with it's own "Options for sphinx_issues" header, so that it's consistent with the rest of the configuration for this project.

Copy link
Contributor Author

@shenxianpeng shenxianpeng Jul 14, 2024

Choose a reason for hiding this comment

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

I've made the changes you suggested 7a5ddfc and 51e4b38.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, trivial fragments aren't displayed anywhere, and their content is disregarded. The contribution guidelines state that they should be empty (but this isn't linted for some reason). This is a way to fool the checks, but it doesn't let one actually tell the end-users what's happened/changed.

I believe that once #12853 is in, it'll be possible to move this to one of packaging/contrib/misc if desired.

@@ -0,0 +1 @@
Integrate ``sphinx-issues`` into the Sphinx config.
Copy link
Member

Choose a reason for hiding this comment

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

By the way, if it's decided that this should go to a change log, it should probably link the integrated project with a hyperlink.

@webknjaz
Copy link
Member

@pradyunsg easy merge?

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

Successfully merging this pull request may close these issues.

[docs] Integrate sphinx-issues into the Sphinx config
5 participants