Skip to content

GH-121970: Use Ruff to check and format the docs tools #122018

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jul 19, 2024
Merged

GH-121970: Use Ruff to check and format the docs tools #122018

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e2026b9
GH-121970: Use Ruff to check and format the docs tools
AA-Turner Jul 19, 2024
a882565
Add ruff-format pre-commit
AA-Turner Jul 19, 2024
13cb3f3
Disable E501
AA-Turner Jul 19, 2024
933095c
Try pre-commit with '--check'
AA-Turner Jul 19, 2024
a9228fe
Remove flake8-errmsg
AA-Turner Jul 19, 2024
efbee68
Revert the version bump (new Lib/test failures)
AA-Turner Jul 19, 2024
7bd87a3
Update comments in .ruff.toml
AA-Turner Jul 19, 2024
65ceaa5
Resolve Alex's notes
AA-Turner Jul 19, 2024
9e95b25
Resolve Hugo's notes
AA-Turner Jul 19, 2024
8fa92b5
Merge branch 'main' into docs/use-ruff
AA-Turner Jul 19, 2024
dc85abe
Reformat with new settings
AA-Turner Jul 19, 2024
a599ef7
Use quoute-style=preserve
AA-Turner Jul 19, 2024
7831456
Preserve even more
AA-Turner Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,21 @@ repos:
rev: v0.3.4
hooks:
- id: ruff
name: Run Ruff on Lib/test/
name: Run Ruff (check) on Doc/
args: [--exit-non-zero-on-fix]
files: ^Doc/
- id: ruff
name: Run Ruff (check) on Lib/test/
args: [--exit-non-zero-on-fix]
files: ^Lib/test/
- id: ruff
name: Run Ruff on Argument Clinic
name: Run Ruff (check) on Argument Clinic
args: [--exit-non-zero-on-fix, --config=Tools/clinic/.ruff.toml]
files: ^Tools/clinic/|Lib/test/test_clinic.py
- id: ruff-format
Copy link
Contributor

Choose a reason for hiding this comment

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

@AA-Turner in pre-commit, it's usually best to put formatters before linters. This is because if there's a rule violation that linter shows, it will then show up as two different broken checks. It's also more important for the post-format version to be checked rather than showing errors on things that then get changed/relocated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, are the pre-commit tools not idempotent? I hadn't realised that order mattered, as I thought we'd configured them to check only in a read-only fashion.

As you can probably tell, I don't use pre-commit personally!

A

Copy link
Member

Choose a reason for hiding this comment

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

The Ruff pre-commit README says:

When running with --fix, Ruff's lint hook should be placed before Ruff's formatter hook, and before Black, isort, and other formatting tools, as Ruff's fix behavior can output code changes that require reformatting.

When running without --fix, Ruff's formatter hook can be placed before or after Ruff's lint hook.

https://github.com/astral-sh/ruff-pre-commit/

Copy link
Contributor

Choose a reason for hiding this comment

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

@AA-Turner oh, I didn't realize. It's so popular that I tend to forget there's people who don't use it. I think I first contributed to pre-commit in 2016…

But yeah, there's science to crafting a config for the pre-commit tool. And there are a few principles I found very useful in this context. Here's how I sort the entries:

  • fast checks first, followed by heavier linters (as in, flake8 goes before pylint, for example, by MyPy would probably be in between)
  • formatters before linters (as in, autopep8 and then flake8)
  • if there are groups of formatters+linters touching different file categories, the entire category can be moved together, no need to put autopep8 and docformatter together as they change different file types

pre-commit itself does not modify things by itself, so you can say it's idempotent in this sense. It's the checks that call other tools that modify files. I think if a check changes files (pre-commit docs sometimes call these “fixers”, not “formatters”), pre-commit will then show that entry as failed (although, it probably relies on the called program's return code). When something changed, it's useful to know what, so it's best to call the tool with --show-diff-on-failure. I think the https://pre-commit.ci web service does this by default (this service is also able to push a commit with the changed files back to pull requests, by the way). Some people only use this CLI flag in CI, while others stick it into the command runner like tox so it's active locally when the contributors run linting.

Feel free to tag me for reviews in future PRs if you need any more insight on this topic.

name: Run Ruff (format) on Doc/
args: [--check]
files: ^Doc/

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 24.4.2
Expand Down
46 changes: 46 additions & 0 deletions Doc/.ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
target-version = "py312" # align with the version in oldest_supported_sphinx
fix = true
output-format = "full"
extend-exclude = [
"includes/*",
"tools/extensions/c_annotations.py",
"tools/extensions/escape4chm.py",
"tools/extensions/patchlevel.py",
"tools/extensions/pyspecific.py",
]

[lint]
preview = true
select = [
"C4", # flake8-comprehensions
"B", # flake8-bugbear
"E", # pycodestyle
"F", # pyflakes
"FA", # flake8-future-annotations
"FLY", # flynt
"FURB",# refurb
"G", # flake8-logging-format
"I", # isort
"LOG", # flake8-logging
"N", # pep8-naming
"PERF",# perflint
"PGH", # pygrep-hooks
"PT", # flake8-pytest-style
"TCH", # flake8-type-checking
"UP", # pyupgrade
"W", # pycodestyle
]
ignore = [
"E501", # We use auto-formatting, so ignore line length errors
]

[format]
preview = true
quote-style = "single"
exclude = [
"tools/extensions/lexers/*",
"tools/extensions/c_annotations.py",
"tools/extensions/escape4chm.py",
"tools/extensions/patchlevel.py",
"tools/extensions/pyspecific.py",
]
120 changes: 73 additions & 47 deletions Doc/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import os
import sys
import time

sys.path.append(os.path.abspath('tools/extensions'))
sys.path.append(os.path.abspath('includes'))

Expand All @@ -30,20 +31,20 @@

# Skip if downstream redistributors haven't installed them
try:
import notfound.extension
import notfound.extension # NoQA: F401
except ImportError:
pass
else:
extensions.append('notfound.extension')
try:
import sphinxext.opengraph
import sphinxext.opengraph # NoQA: F401
except ImportError:
pass
else:
extensions.append('sphinxext.opengraph')


doctest_global_setup = '''
doctest_global_setup = """
try:
import _tkinter
except ImportError:
Expand All @@ -53,7 +54,7 @@
import warnings
warnings.simplefilter('error')
del warnings
'''
"""

manpages_url = 'https://manpages.debian.org/{path}'

Expand All @@ -63,7 +64,8 @@

# We look for the Include/patchlevel.h file in the current Python source tree
# and replace the values accordingly.
import patchlevel
import patchlevel # NoQA: E402

version, release = patchlevel.get_version_info()

rst_epilog = f"""
Expand Down Expand Up @@ -298,7 +300,8 @@

# Disable Docutils smartquotes for several translations
smartquotes_excludes = {
'languages': ['ja', 'fr', 'zh_TW', 'zh_CN'], 'builders': ['man', 'text'],
'languages': ['ja', 'fr', 'zh_TW', 'zh_CN'],
'builders': ['man', 'text'],
}

# Avoid a warning with Sphinx >= 4.0
Expand All @@ -319,30 +322,32 @@
'collapsiblesidebar': True,
'issues_url': '/bugs.html',
'license_url': '/license.html',
'root_include_title': False # We use the version switcher instead.
'root_include_title': False, # We use the version switcher instead.
}

if os.getenv("READTHEDOCS"):
html_theme_options["hosted_on"] = '<a href="https://about.readthedocs.com/">Read the Docs</a>'
if os.getenv('READTHEDOCS'):
html_theme_options['hosted_on'] = (
'<a href="https://about.readthedocs.com/">Read the Docs</a>'
)

# Override stylesheet fingerprinting for Windows CHM htmlhelp to fix GH-91207
# https://github.com/python/cpython/issues/91207
if any('htmlhelp' in arg for arg in sys.argv):
html_style = 'pydoctheme.css'
print("\nWARNING: Windows CHM Help is no longer supported.")
print("It may be removed in the future\n")
print('\nWARNING: Windows CHM Help is no longer supported.')
print('It may be removed in the future\n')

# Short title used e.g. for <title> HTML tags.
html_short_title = f'{release} Documentation'

# Deployment preview information
# (See .readthedocs.yml and https://docs.readthedocs.io/en/stable/reference/environment-variables.html)
repository_url = os.getenv("READTHEDOCS_GIT_CLONE_URL")
repository_url = os.getenv('READTHEDOCS_GIT_CLONE_URL')
html_context = {
"is_deployment_preview": os.getenv("READTHEDOCS_VERSION_TYPE") == "external",
"repository_url": repository_url.removesuffix(".git") if repository_url else None,
"pr_id": os.getenv("READTHEDOCS_VERSION"),
"enable_analytics": os.getenv("PYTHON_DOCS_ENABLE_ANALYTICS"),
'is_deployment_preview': os.getenv('READTHEDOCS_VERSION_TYPE') == 'external',
'repository_url': repository_url.removesuffix('.git') if repository_url else None,
'pr_id': os.getenv('READTHEDOCS_VERSION'),
'enable_analytics': os.getenv('PYTHON_DOCS_ENABLE_ANALYTICS'),
}

# This 'Last updated on:' timestamp is inserted at the bottom of every page.
Expand Down Expand Up @@ -388,15 +393,15 @@

latex_elements = {
# For the LaTeX preamble.
'preamble': r'''
'preamble': r"""
\authoraddress{
\sphinxstrong{Python Software Foundation}\\
Email: \sphinxemail{[email protected]}
}
\let\Verbatim=\OriginalVerbatim
\let\endVerbatim=\endOriginalVerbatim
\setcounter{tocdepth}{2}
''',
""",
# The paper size ('letter' or 'a4').
'papersize': 'a4',
# The font size ('10pt', '11pt' or '12pt').
Expand All @@ -407,30 +412,52 @@
# (source start file, target name, title, author, document class [howto/manual]).
_stdauthor = 'Guido van Rossum and the Python development team'
latex_documents = [
('c-api/index', 'c-api.tex',
'The Python/C API', _stdauthor, 'manual'),
('extending/index', 'extending.tex',
'Extending and Embedding Python', _stdauthor, 'manual'),
('installing/index', 'installing.tex',
'Installing Python Modules', _stdauthor, 'manual'),
('library/index', 'library.tex',
'The Python Library Reference', _stdauthor, 'manual'),
('reference/index', 'reference.tex',
'The Python Language Reference', _stdauthor, 'manual'),
('tutorial/index', 'tutorial.tex',
'Python Tutorial', _stdauthor, 'manual'),
('using/index', 'using.tex',
'Python Setup and Usage', _stdauthor, 'manual'),
('faq/index', 'faq.tex',
'Python Frequently Asked Questions', _stdauthor, 'manual'),
('whatsnew/' + version, 'whatsnew.tex',
'What\'s New in Python', 'A. M. Kuchling', 'howto'),
('c-api/index', 'c-api.tex', 'The Python/C API', _stdauthor, 'manual'),
(
'extending/index',
'extending.tex',
'Extending and Embedding Python',
_stdauthor,
'manual',
),
(
'installing/index',
'installing.tex',
'Installing Python Modules',
_stdauthor,
'manual',
),
(
'library/index',
'library.tex',
'The Python Library Reference',
_stdauthor,
'manual',
),
(
'reference/index',
'reference.tex',
'The Python Language Reference',
_stdauthor,
'manual',
),
('tutorial/index', 'tutorial.tex', 'Python Tutorial', _stdauthor, 'manual'),
('using/index', 'using.tex', 'Python Setup and Usage', _stdauthor, 'manual'),
('faq/index', 'faq.tex', 'Python Frequently Asked Questions', _stdauthor, 'manual'),
(
'whatsnew/' + version,
'whatsnew.tex',
"What's New in Python",
'A. M. Kuchling',
'howto',
),
]
# Collect all HOWTOs individually
latex_documents.extend(('howto/' + fn[:-4], 'howto-' + fn[:-4] + '.tex',
'', _stdauthor, 'howto')
for fn in os.listdir('howto')
if fn.endswith('.rst') and fn != 'index.rst')
latex_documents.extend(
('howto/' + fn[:-4], 'howto-' + fn[:-4] + '.tex', '', _stdauthor, 'howto')
for fn in os.listdir('howto')
if fn.endswith('.rst') and fn != 'index.rst'
)

# Documents to append as an appendix to all manuals.
latex_appendices = ['glossary', 'about', 'license', 'copyright']
Expand Down Expand Up @@ -458,8 +485,7 @@
'test($|_)',
]

coverage_ignore_classes = [
]
coverage_ignore_classes = []

# Glob patterns for C source files for C API coverage, relative to this directory.
coverage_c_path = [
Expand All @@ -476,7 +502,7 @@
# The coverage checker will ignore all C items whose names match these regexes
# (using re.match) -- the keys must be the same as in coverage_c_regexes.
coverage_ignore_c_items = {
# 'cfunction': [...]
# 'cfunction': [...]
}


Expand Down Expand Up @@ -534,10 +560,10 @@
# mapping unique short aliases to a base URL and a prefix.
# https://www.sphinx-doc.org/en/master/usage/extensions/extlinks.html
extlinks = {
"cve": ("https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-%s", "CVE-%s"),
"cwe": ("https://cwe.mitre.org/data/definitions/%s.html", "CWE-%s"),
"pypi": ("https://pypi.org/project/%s/", "%s"),
"source": (SOURCE_URI, "%s"),
'cve': ('https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-%s', 'CVE-%s'),
'cwe': ('https://cwe.mitre.org/data/definitions/%s.html', 'CWE-%s'),
'pypi': ('https://pypi.org/project/%s/', '%s'),
'source': (SOURCE_URI, '%s'),
}
extlinks_detect_hardcoded_links = True

Expand Down
Loading
Loading