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.

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
12 changes: 10 additions & 2 deletions .pre-commit-config.yaml
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 (lint) on Doc/
args: [--exit-non-zero-on-fix]
files: ^Doc/
- id: ruff
name: Run Ruff (lint) on Lib/test/
args: [--exit-non-zero-on-fix]
files: ^Lib/test/
- id: ruff
name: Run Ruff on Argument Clinic
name: Run Ruff (lint) 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
45 changes: 45 additions & 0 deletions Doc/.ruff.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
target-version = "py312" # Align with the version in oldest_supported_sphinx
fix = true
output-format = "full"
line-length = 79
extend-exclude = [
"includes/*",
# Temporary exclusions:
"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", # Ignore line length errors (we use auto-formatting)
]

[format]
preview = true
quote-style = "preserve"
docstring-code-format = true
exclude = [
"tools/extensions/lexers/*",
]
118 changes: 83 additions & 35 deletions Doc/conf.py
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,13 +31,13 @@

# 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:
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,11 +322,13 @@
'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>'
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
Expand All @@ -337,17 +342,21 @@

# 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")
is_deployment_preview = os.getenv("READTHEDOCS_VERSION_TYPE") == "external"
repository_url = os.getenv("READTHEDOCS_GIT_CLONE_URL", "")
repository_url = repository_url.removesuffix(".git")
html_context = {
"is_deployment_preview": os.getenv("READTHEDOCS_VERSION_TYPE") == "external",
"repository_url": repository_url.removesuffix(".git") if repository_url else None,
"is_deployment_preview": is_deployment_preview,
"repository_url": repository_url or 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.
html_time = int(os.environ.get('SOURCE_DATE_EPOCH', time.time()))
html_last_updated_fmt = time.strftime('%b %d, %Y (%H:%M UTC)', time.gmtime(html_time))
html_last_updated_fmt = time.strftime(
'%b %d, %Y (%H:%M UTC)', time.gmtime(html_time)
)

# Path to find HTML templates.
templates_path = ['tools/templates']
Expand Down Expand Up @@ -407,30 +416,70 @@
# (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 +507,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 +524,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
22 changes: 11 additions & 11 deletions Doc/tools/check-warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"""
Check the output of running Sphinx in nit-picky mode (missing references).
"""

from __future__ import annotations

import argparse
Expand Down Expand Up @@ -206,7 +207,9 @@ def annotate_diff(


def fail_if_regression(
warnings: list[str], files_with_expected_nits: set[str], files_with_nits: set[str]
warnings: list[str],
files_with_expected_nits: set[str],
files_with_nits: set[str],
) -> int:
"""
Ensure some files always pass Sphinx nit-picky mode (no missing references).
Expand Down Expand Up @@ -252,17 +255,11 @@ def fail_if_new_news_nit(warnings: list[str], threshold: int) -> int:
"""
Ensure no warnings are found in the NEWS file before a given line number.
"""
news_nits = (
warning
for warning in warnings
if "/build/NEWS:" in warning
)
news_nits = (warning for warning in warnings if "/build/NEWS:" in warning)

# Nits found before the threshold line
new_news_nits = [
nit
for nit in news_nits
if int(nit.split(":")[1]) <= threshold
nit for nit in news_nits if int(nit.split(":")[1]) <= threshold
]

if new_news_nits:
Expand Down Expand Up @@ -311,7 +308,8 @@ def main(argv: list[str] | None = None) -> int:
exit_code = 0

wrong_directory_msg = "Must run this script from the repo root"
assert Path("Doc").exists() and Path("Doc").is_dir(), wrong_directory_msg
if not Path("Doc").exists() or not Path("Doc").is_dir():
raise RuntimeError(wrong_directory_msg)

with Path("Doc/sphinx-warnings.txt").open(encoding="UTF-8") as f:
warnings = f.read().splitlines()
Expand Down Expand Up @@ -339,7 +337,9 @@ def main(argv: list[str] | None = None) -> int:
)

if args.fail_if_improved:
exit_code += fail_if_improved(files_with_expected_nits, files_with_nits)
exit_code += fail_if_improved(
files_with_expected_nits, files_with_nits
)

if args.fail_if_new_news_nit:
exit_code += fail_if_new_news_nit(warnings, args.fail_if_new_news_nit)
Expand Down
10 changes: 7 additions & 3 deletions Doc/tools/extensions/glossary_search.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@
logger = logging.getLogger(__name__)


def process_glossary_nodes(app: Sphinx, doctree: nodes.document, _docname: str) -> None:
def process_glossary_nodes(
app: Sphinx,
doctree: nodes.document,
_docname: str,
) -> None:
if app.builder.format != 'html' or app.builder.embedded:
return

Expand All @@ -34,15 +38,15 @@ def process_glossary_nodes(app: Sphinx, doctree: nodes.document, _docname: str)
rendered = app.builder.render_partial(definition)
terms[term.lower()] = {
'title': term,
'body': rendered['html_body']
'body': rendered['html_body'],
}


def write_glossary_json(app: Sphinx, _exc: Exception) -> None:
if not getattr(app.env, 'glossary_terms', None):
return

logger.info(f'Writing glossary.json', color='green')
logger.info('Writing glossary.json', color='green')
dest = Path(app.outdir, '_static', 'glossary.json')
dest.parent.mkdir(exist_ok=True)
dest.write_text(json.dumps(app.env.glossary_terms), encoding='utf-8')
Expand Down
2 changes: 1 addition & 1 deletion Doc/tools/extensions/lexers/asdl_lexer.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class ASDLLexer(RegexLexer):
# Keep in line with ``builtin_types`` from Parser/asdl.py.
# ASDL's 4 builtin types are
# constant, identifier, int, string
('constant|identifier|int|string', Name.Builtin),
("constant|identifier|int|string", Name.Builtin),
(r"attributes", Name.Builtin),
(
_name + _text_ws + "(=)",
Expand Down
Loading